1
void executeRequests() {

    ...

    for (Request request in requests) {
        if (request is Type1Request) {
            ...
        } else if (request is Type2Request) {
            ...
        } else if (...) {

        } ...
    }

}

So basically I have a method that does some common process for a list of requests and then iterate through this list and based on the request type (its specific class) it does some specific processing.

One might argue that I should separate each if else block into a separate classes that has an execute method and then just call the execute inside the for loop because this breaks the Separation of concerns pattern. And I agree! I just don't really know why.

In this case, lets assume that I will never reuse those classes or private functions (or however I decide to refactor). Some people argue that this way is easier to read because you don't have to jump to different files or function to understand everything.

So my question is, why is this piece of code bad?

Thank you.

Rodrigo Ruiz
  • 4,248
  • 6
  • 43
  • 75
  • If you want a detailed explanation behind these concepts, I highly recommend that you read [Clean Code by Robert C. Martin](http://www.amazon.ca/Clean-Code-Handbook-Software-Craftsmanship/dp/0132350882). You can also take a look at [this blog post](http://blog.8thlight.com/uncle-bob/2014/05/08/SingleReponsibilityPrinciple.html) from the same author. – Etienne Maheu Mar 03 '15 at 22:33

2 Answers2

1

Simply put, because adding a Type3Request object in your code forces you to edit existing code. Every time you change existing code, you should test all the related modules in your application to ensure that no regression bugs appear in your code. In javascript or other loosely-typed languages, it is very easy to overwrite global data from an if branch. In most strongly-typed languages you can generally avoid this issue, but there is still a possibility of overwriting local data. This means that every time you change this executeRequests method, you have to run all possible requests in your application to ensure that you have not introduced a new issue. I want to make clear that I said all requests, not all requests types. This is potentially thousands of tests to run even if you have only a few request types.

On the other side, keeping this code inside different classes as a polymorphic method ensures that it cannot changes data from the executeRequests method as well as its owner class. This enables you to test only calls related to the type that you are changing instead with the same confidence level. In other words, it reduces the amount of bugs in your code.

By using polymorphism, all the "specific code" that needs to be executed for this request will not only be encapsulated into the Type3Request object, but will actually come with this object. There is no other code file to change in order to add a new request type.

This means that you can provide new request types dynamically as plugins, simply by adding a dll in a folder and without recompiling the existing code. If you do no want to go that far, it still gives you the ability to better separate your workload between all team members by avoiding merges to that executeRequests method.

It also makes it simpler to test code by keeping tightly coupled business logic together. Instead of having to get your request to go through the entire execution process just to test this type specific code, you can just call the method directly from the request object and assert the results.

All of this ensures that if an architecture change happens in your project, you know that there is only one place to look for request related code: the related request type class. If you let this code leak into other classes, you might miss some instances of this code when estimating the complexity of changes or even worse, leave it behind after a large refactor which might trigger bugs.

Etienne Maheu
  • 3,235
  • 2
  • 18
  • 24
  • What is the problem of editing existent code? Someone might argue that the specific code is encapsulated in that "else if" block. I'm not really worried about language specifics, lets assume I'm using an interpreted language. The merges is a good argument, but I can still split the work as different request types, then the merge would just be to accept both changes. Some people (not me) think testing is a waste of time, so I don't wanna use that as an argument. In any case, why would it be simpler to test? I can still create a test method for each "else if". – Rodrigo Ruiz Mar 03 '15 at 21:52
  • That last argument seems like in favor of the "if else"'s, by keeping all the code in one single file makes it easier find all the request related code. – Rodrigo Ruiz Mar 03 '15 at 21:53
  • I have improved the answer to cover your comments. – Etienne Maheu Mar 03 '15 at 22:24
  • Would you refactor this switch statement? http://stackoverflow.com/questions/28840957/how-to-unit-test-a-method-whose-side-effect-is-to-call-other-method/28841965#28841965 – Rodrigo Ruiz Mar 04 '15 at 01:13
0

What action is associated with a particular request is of no concern to the object that executes a particular request because of some external trigger. The concern (handling the consequence) belongs to the request. You thus separate it from the executor because it is there incorrectly. Something internal (an action of certain type) is "outsourced" to other class and the class is dominating that decision, which is a liability and coupling.

It's not a question of why you should optionally do this. It's opposite..it is how could it be any other way. If we do proper object oriented programming.

Earl Grey
  • 7,426
  • 6
  • 39
  • 59
  • All true, but I want to know the specific reason on why this coupling of concerns is bad. Why is object oriented programming better? – Rodrigo Ruiz Mar 03 '15 at 21:55
  • Because you want your applications to have a predictable reliable architecture. Coupling multiplies (I don;t even know the rate..probably exponentialy) possible paths for failure, makes the code more difficult to debug, unit test, comprehend and change. – Earl Grey Mar 03 '15 at 21:57