Get rid of long/Complex if..else statements using Chain of Responsibility?

Question

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.

1. Retrieve the encrypted code from request (could be in Form/X-Header/Query)
2. Decrypt it. (Results in an JSON string)
3. Deserialize to a dynamic type
4. Determine the type of request, could be login/register/activate
5. Validate request
6. Load user subscription
7. Load User entity
8. If Req.Type = Login -> Login using FormsAuth
9. If Req.Type = Register -> Create user
10. 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.

Answer 1

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

1. Is the request type in my set of permitted types?
2. If yes, invoke the corresponding class
3. 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

1. Retrieve the encrypted code from request (could be in Form/X-Header/Query)
2. Decrypt it. (Results in an JSON string)
3. Deserialize to a dynamic type
4. Determine the type of request, could be login/register/activate
5. if type in set-of-allowed-types then request-type = new type-specific-class else fail.
6. if not request-type.validate() then fail
7. 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.

Answer 2

The question you linked to gives an answer which will probably apply to your situation as well: introducing CoR will have a a high risk of making complicated things even more complicated. "CoR pattern" is fine if you want to modify the chain sequence at run time, which I guess is not the case in your situation above.

So my advice here is:

• have a method for each of the first seven steps (with a return value if the step fails).

• group the calls of that methods together (perhaps all 7 calls, or you first group intermediate steps together). May result in a method like

   StatusCode PreprocessRequest()
   {
      if(!RetrieveEncryptedCodeFromRequest())
          return StatusCode.RetrieveEncryptedFailed;
  
      if(!Decrypt())
         return StatusCode.DecryptFailed;
  
      //...
      return StatusCode.OK;
    }
  

Then call that method like

  ProcessRequest processor = new ProcessRequest();
  // ...
  StatusCode state = processor.PreprocessRequest();
  if(state==StatusCode.OK)
  {
      Request req = processor.GetRequest();
      HandleRequest(req);
  }
  else
  {
       LogError(state);
  }

(Of course, the details may look different in your case, especially the error handling, but I hope you get the idea.)

So, here is the gist: try to make your "if"s less complex by simply grouping things together into methods, make the error handling uniform on each level, make sure to have parts of a method all on the same level of abstraction (otherwise introduce another intermediate method). I guess that will help you to improve the code to a degree where the "complex if/else statements" vanish.

Answer 3

In Chain of responsibility, a processing object which is unable to process a given entry will pass it to the next processing object.

For example, if you're building a system which will process messages from a hardware device, one processing object (PO₁) may be able to handle the control messages, another one (PO₂) — errors, and the last one (PO₃) — data flow.

If a message is neither a control message, nor an error, it is passed by the PO₁ to PO₂ and by the PO₂ to the PO₃ who will either process the message or throw an exception, because the message is not recognized.

Chain of responsibility pattern is convenient when you need to process something uniform, without modifying it.

If you use Chain of responsibility pattern, it would mean that:

1. Every processing object will be required to handle or pass a bunch of objects (probably grouped into one huge object) with information about sessions, request, etc. When one should pass so many information from a method to another, this is a sign that something may be wrong.

2. Some processing objects will modify the context. This is not something current in Chain of responsibility pattern.

The current flow in your question looks like an ordinary application flow. There is no need to use Chain of responsibility for that. For example, in this situation:

1. Check if the file exists and is readable.

2. Open the file.

3. Read its contents.

4. Check the header to ensure that the format is correct.

5. Verify the CRC.

6. Parse contents.

7. Compute the statistics from the parsed contents.

8. Return the statistical results.

the workflow is very similar, i.e. some of the steps may indicate that others shouldn't be done: for example if contents can't be parsed, it wouldn't make sense to compute the statistics, or if the header is invalid, there is no need to verify the CRC. However, Chain of responsibility pattern doesn't fit well here, because different steps use different data, and can modify it (or create additional data).

Answer 4

Don't forget to consider the different responsibilities here.
You have

1. an encrypted request object that needs to be decrypted
2. a JSON deserializer
3. something that determines the request type
4. a request validator
5. some data access stuff
6. and something that chooses the next step to take
   
   I would not advise you to put all that stuff in one IHttpHandler (SRP violation). Split if off along responsability lines and use composition in your handler. That will probably make it easier to read and you can test the different components in isolation.