A couple of years ago I was working on a project in which a significant portion (say 30%) of the codebase was legacy code. By which I mean that it was written by a chap who had just left the team. He was a ‘clever’ programmer – his code did the job, and the rest of us were afraid to touch it. The coding style (this was a Java project) was intensely algorithmic. It involved a small number of huge classes, each method of which took parameters to configure their behaviour. No-one could touch it because the inter-relationships between the (large) pieces were too opaque, and none of us was brave enough to even begin the process of refactoring towards a more ‘object-oriented’ solution. (One thing that would now ring alarm bells with me: the code failed Mike Feathers’ test, in that it was well-nigh impossible to write unit tests for any piece of the code. In fact, most of the classes couldn’t be instantiated outside of this sub-system’s highly complex nutrient soup.)

Then one day I was looking through the tests for this code. They were very similar in style to the code they were testing, and the following pattern was oft repeated:

public void test_x() {
  doTestX(null, "something", null, null);
  doTestX("sponge", null, null, null);
  doTestX("sponge", null, "specialvalue", null);

The test is completely unreadable! The doTestX() method is no better, because it is littered with if statements where it tries to work out exactly how to call the method under test. So as an exercise, I made a copy of doTestX() for each different combination of nulls in the calling tests. After further refactoring and simplification (eg. to remove redundant parameters), I found that we no longer had any nulls, and in fact we no longer had any if statements either. At that point I decided that null arguments should be banned.

It was also around this time that I found the example described in avoid boolean parameters. Eventually I realised that it’s not strictly the null or boolean parameters that are the problem – it’s the conditionals required to deal with them. The existence of if statements in this code signals duplication, because the caller already knows which path it wants the callee to follow. In a misguided attempt to remove duplication, the programmer had brought together functionality that didn’t belong together, and in so doing had created more duplication.

Therefore it seems to me that there are two kinds of conditional statement in a codebase: The first kind tests an aspect of the running system’s external environment (did the user push that button? does the database hold that value? does that file exist?). And the second kind tests something that some other part of the system already knows. Let’s ban the second kind…

[Update: if we can indeed eliminate these duplicate conditional statements, it has some interesting consequences for system architectures…]


5 thoughts on “if…

  1. Good point. Let us stamp out procedural programming wrapped in OO classes for good.

    Quite interesting, today a junior developer asked me the following question:

    “When is a class to big?”

    My answer:

    “When the class is doing too much.”

    Then I started to explain the concept of Cohesion and Responsibility-Driven Design (RDD) to the developer.

    It is just mind boggling how little developers, even senior developers in today’s workforce know about Object-Orientation.

  2. Pingback: the anti-if campaign « silk and spinach

  3. Thanks for the reminder. Checked my projects code base and instantly found some of this slipped into it, now tagged for refactoring.

  4. Pingback: If… « silk and spinach

  5. Pingback: Conditionals on the edge « silk and spinach

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s