Design Patterns – Using Chain of Responsibility to Simplify Complex if..else Statements

asp.netcdesign-patternsrefactoring

I've an HttpHandler, which allows users to login to a system by passing in an encrypted code.

Inside the ProcessRequest it performs quite a few steps.

Retrieve the encrypted code from request (could be in Form/X-Header/Query)
Decrypt it. (Results in an JSON string)
Deserialize to a dynamic type
Determine the type of request, could be login/register/activate
Validate request
Load user subscription
Load User entity
If Req.Type = Login -> Login using FormsAuth
If Req.Type = Register -> Create user
If Req.Type = Register -> Send Activation email

This goes on. Currently this is something like a long if/else statement which I'm trying to refactor.
I'm thinking of using the Chain Of Responsibility pattern to convert these steps in if/else to a chain which executes sequentially.
However, this will be different from original CoR as only last few steps will actually "handle" the request, first steps will just do some pre-processing and make stage for last steps to perform it's job.

My main problem is, different steps in the chain will work on different input data. First few steps (Decryption, Deserialization, etc) will work on strings while latter half should ideally work on deserialized dynamic object. I don't like the way it sounds.

Am I trying to fit a round lid to an square bottle? Is this not a scenario where CoR can/should be applied? Are there any other patterns/strategies I could use to solve such scenarios.

I thought of using the decorator pattern as well, but that doesn't suite me very well, as I'd like to be able to switch certain steps in and out (ex: email activation) for some scenarios.

NOTE: I've seen this question as well, but not sure if it answers my question.

Best Answer

Your ProcessRequest method is doing too much in one block of code. Chain of Responsibility is an inappropriate pattern because some of these steps are mandatory and some are alternatives.

Look at the last 3 steps - 8 to 10. Your main method should not care what has to be done for each individual type of request. All that should be happening here is

Is the request type in my set of permitted types?
If yes, invoke the corresponding class
If no, handle the error.

That's going to be a very short block of code. It does require you, somewhere else in your code, to create your request-type-specific class (and subclasses) and the set to contain them, but the benefit is that this block of code in your main method need never change.

Step 5 should be a method of the type-specific class.

Steps 6 and 7 can probably be encapsulated in a User object but also only mean something for existing users, so the User-object handling stuff should go inside the type-specific class.

So we've already reduced the code to

Retrieve the encrypted code from request (could be in Form/X-Header/Query)
Decrypt it. (Results in an JSON string)
Deserialize to a dynamic type
Determine the type of request, could be login/register/activate
if type in set-of-allowed-types then request-type = new type-specific-class else fail.
if not request-type.validate() then fail
request-type.dostuff()

See? No patterns involved. If-then-else chains are a bad smell, yes, but they can usually be replaced with a set of valid options or a switch statement. Try that simple approach before going all pattern-mad.

Related Solutions

Java: How to implement a step builder for which the order of setters doesn’t matter

You seem to have two different requirements, based on the method calls you provided.

Only one (required) engine, only one (required) transmission, and only one (optional) stereo.
One or more (required) engines, one or more (required) transmissions, and one or more (optional) stereos.

I think the first issue here is that you don't know what you want the class to do. Part of that is that it's not known what you want the built object to look like.

A car can only have one engine and one transmission. Even hybrid cars only have one engine (perhaps a GasAndElectricEngine)

I'll address both implementations:

public class CarBuilder {

    public CarBuilder(Engine engine, Transmission transmission) {
        // ...
    }

    public CarBuilder setStereo(Stereo stereo) {
        // ...
        return this;
    }
}

and

public class CarBuilder {

    public CarBuilder(List<Engine> engines, List<Transmission> transmission) {
        // ...
    }

    public CarBuilder addStereo(Stereo stereo) {
        // ...
        return this;
    }
}

If an engine and transmission are required, then they should be in the constructor.

If you don't know what engine or transmission is required, then don't set one yet; it's a sign that you're creating the builder too far up the stack.

C# – Refactoring Many Else If Statements

Here's what I would start with given the information you've provided.

Create an interface like so:

interface LineProcessor<E> {
  boolean Process(Record record, Line line); 
}

Let's assume Record is a propery bag for now. Don't get hung up on this, I'm keeping it simple for demonstration purposes.

class Record {
  public Date dob { get; set; }
  public String gender { get; set; }
  public String refNumber { get; set; }
  // ...
}

Sorry if the syntax is not right for C#. If you aren't sure what I'm getting at, I'll clarify.

Then create a list of instances of LineParser: one for type of line. Then, you write a loop (python/pseudocode):

for line in pdf:
  obj = process(record, line)

def process(record, line):
  for handler in processors:
    if handler.process(record, line): return
  else:
    unknown.add(line)

Now the implementation of one of these might look like so:

class DobProcessor implements LineProcessor {
  boolean process(Record record, Line line) {
    if (IsDob(line)) {
      record.dob = ParseDob(line);
      return true;
    } else {
      return false;
    }
  }

  Date ParseDob(Line line) {
    //...
  }

  boolean IsDob(Line line) {
    //...
  }
}

This should make the code more manageable. Instead of a large if statement, you will have a number of classes where each one is specific to a case. This not only organizes the code, it means you can add or remove cases without touching any of the code around other cases.

One other thing is that now that the processes interface is down to a single method, you can actually start thinking of this as more of a function pointer/lambda so you can dispense with the interface if you so desire.

Best Answer

Related Solutions

Java: How to implement a step builder for which the order of setters doesn’t matter

C# – Refactoring Many Else If Statements

Related Topic