if-else trees vs SOLID principles

Introduction

If you have read my previous articles, or know me personally, you must know that the one kind of bad code I hate the most is if-else trees. In the first article of this weblog, I talked about if-else trees in general, exposing some general ideas about how to avoid them, remove them from the code or why they are bad, describing them as a code smell.

Today, as I have just finished writing a series of articles in French about the SOLID principles, I am going to focus on the maintainability aspect of if-else trees, and make them fight against two of the SOLID principles: the Single Responsibility Principle and the Open/Closed Principle. This will allow me to introduce some important rules to know about if-else trees.

 Definitions

  • maintainability
    I call “maintainability” the easiness of making changes in an application. This includes every kind of changes such as bugfixes, new features, changes in existing behaviors, etc. and can be considered as a synonym of scalability. Although several metrics exist, I will keep this definition at a subjective level.
  • if-else tree
    An “if-else tree” is a portion of code composed of nested if-else statements. This can be widen to switch statements and sometimes even to loops. The if-else tree is generally big – I mean BIG.
    There are at least two kinds of if-else trees:
    • the one-case-one-branch tree, an if-else tree generally built step-by-step, where adding a new case, a new feature, means adding a new conditional block. Often the developer does not try to avoid condition or code duplication and just adds what he has to at the right place. This kind of tree grows bigger and bigger. From a maintainability point of view, a one-case-one-branch tree is not too difficult to understand and refactor into a better design, as each case often being quite well defined in the tree.
    • the factorized tree, an if-else tree built with the objective of being optimized. All the cases were known from the beginning and the developer factorized the corresponding conditions to try and avoid condition and code duplication. This is definitely the worst kind of tree from a maintainability point of view, as all the cases are merged together into an arcane entity.

Rule #1 : a factorized if-else tree is harder to maintain than a one-case-one-branch tree

  • SOLID principles
    Coined by Robert C. Martin, this acronym stands for five principles of object-oriented design that, when applied together, make an application design and code more maintainable.

Description of the example and disclaimer

This article is illustrated with an example from the famous open-source project ps3mediaserver. Please note that I have no relation with this project. It was picked-up randomly just because it is coded with an object-oriented language (Java) and contains an interesting if-else tree. My purpose here is not to blame anybody but only to illustrate the fact that if-else trees are bad regarding maintainability. Also remember that being badly designed regarding maintainability does not mean that the application does not do what it is expected to do.

Ps3mediaserver is a Java UPnP media server dedicated to the Playstation 3. The class we are going to use here as an example is net.pms.network.RequestV2. This is the central class of the HTTP part of the server. The if-else tree we are going to have a look at is located in the answer function. I kindly invite you to quickly read through the if-else tree before reading this article any further. You don’t have to understand everything, just get yourself familiar with the tree.

Single Responsibility Principle vs if-else trees

A typical if-else tree has several responsibilities. When you are looking at an if-else tree, you are very often seeing a god object. Indeed, as the if-else tree knows many things on “when to do”, with all its conditions, it might very likely know many things on “how to do” as well.

Rule #2 : When you are looking at an if-else tree, you are very often seeing a god object

The example if-else tree is definitely a one-case-one-branch tree (OCOB tree), the kind of if-else tree where every case has its own conditional block. When dealing with an OCOB tree, one can be pretty sure that the Single Responsibility Principle is not applied. Indeed, OCOB trees are by definition an aggregate of responsibilities packed together using an if-else tree.

With our example, the description comment of the class is nearly sufficient : “This class handles all forms of incoming HTTP requests by constructing a proper HTTP response.” In fact this is the if-else tree that “handles all forms of incoming HTTP requests”, and each if is for one of them.

Robert C. Martin introduces the Single Responsibility Principle with this statement:

There should never be more than one reason for a class to change.

Here there is a responsibility – a reason for changing the class – associated with each block of the tree. From the beginning of the example tree, we have:

  • outputting a page to the HTML console
  • retrieving a file in general
  • retrieving a thumbnail
  • retrieving a subtitle
  • retrieving a regular file
  • notifying plugins
  • generating a response
  • outputting a picture
  • browsing files

…and the Single Responsibility Principle is obviously not applied here. This is not the point of this article but in this particular situation, one could have used the Chain of Responsibility pattern, thus distributing the responsibilities among the different handlers, each of them responsible for only one kind of request. This is what is done to handle HTTP requests in about every existing framework in any language. For Java, readers might want to have a look at org.apache.http as an example implementation.

Rule #3 : A one-case-one-branch tree can often be split using a chain of responsibility

Open/Closed Principle vs if-else trees

If-else trees often break the Open/Closed Principle, creating excessive coupling and maintainability issues. If-else trees break the Open/Closed Principle when some conditions are based on the knowledge of the implementation details instead of based on abstraction. This can be a direct and heavy type checking, or something more subtle based on the knowledge of a particular post-condition, etc. The latter is often found with a comment like “Being in this situation means that the actual handler was XYZ” or “As xyz was empty before the call, we can tell for sure that Foo did blabla…“, which expresses the developer’s guilt-feelings for writing such a condition; and is definitely a code smell.

Rule #4 : In an if-else tree, an over-commented block of code might be the expression of guilt-feelings from the developer, indicating a potential code/design problem.

In the example we can find a couple of such comments, for example:

//hacky stuff. replace the png icon by a jpeg one. Like mpeg2 remux,
//really need a proper format compatibility list by renderer

or

// with the new parser, files are parsed and analyzed *before* creating the DLNA tree,
// every 10 items (the ps3 asks 10 by 10),
// so we do not know exactly the total number of items in the DLNA folder to send
// (regular files, plus the #transcode folder, maybe the #imdb one, also files can be
// invalidated and hidden if format is broken or encrypted, etc.).
// let’s send a fake total size to force the renderer to ask following items

Definitely a design issue here as the RequestV2 class has to know hyper-detailed details on the implementation of other classes to work properly.

Do not build if-else trees, ever!

Before building an if-else tree, you should always ask yourself whether there is a design issue somewhere. You should never feel like you have to build such a tree as this nearly always implies that the class will know too much of something. This can be too many responsibilities or too many implementation details, as we’ve just seen in this article, but this can also be a too big business : an apparently unique responsibility hiding lots of sub-responsibilities that can be split.

Rules #5 : An if-else tree always knows too much

Conclusion

Thank you for reading this article, which is not the last one on the subject of code maintainability/scalability/evolutionability. Indeed, although this need of code maintainability has always been around, the cloud computing/agile development era brings it to the forefront. In a world where more and more,we need to develop new features and make existing ones evolve at the speed of light, maintainability will become the number-one rule of every company, and if-else trees do not fit. They simply do not belong to this new era.

Further readings

I’m feeling lazy today, so a pure wikipedia “further readings” section:

Leave a Reply