We are the Dev Teams of
  • brands
  • ebay_main
  • ebay
  • mobile
<
>
BLOG

An Exercise in Unconditional Programming

by Mike Zimmermann
in Tutorials

Are conditional statements like if or switch really always necessary? How can we determine when they are, and when not and how can we avoid using bad ifs.

In an exercise to abolish ifs, I refactored a class of the mobile.de code base. 

Introduction

The intention of this post is to have a look at conditional statements like IF or SWITCH and see if they are really necessary and how they can be avoided.

Instead of looking at academic samples, I undertook as an exercise the refactoring of a class of the mobile.de code base to see how far I could get to abolish IFs. This a report of outcome of the refactoring and how control IFs were abolished.

Arrow-Head Pattern

Deeply nested blocks at first glance look intimidating. They display what is known as the Arrow Head Pattern.

if ( ... ) {
       if ( ... ) {
              if ( ... ) {

                    ...
              } else if ( ... )  {
                     if ( ... ) {
                         //--> do something
                     }
              }
       }
}

In general, such code is difficult to read and maintain, often violating software design principles such as single responsibility, open closed principle. Excessive use of IF is usually a sign of missing design in code and shows a tendency to write code in a top down procedural manner.  

Adding features to deeply nested code, usually leads to more IFs, increasing the complexity.

Michael Feathers:

Some of the worst code I've ever seen is a nightmare of nested conditions with odd bits of work interspersed within them…. the real problem with control structures is that they are often mixed with the work” (http://michaelfeathers.typepad.com/michael_feathers_blog/2013/11/unconditional-programming.html)

Anti-IF campaign

(http://www.antiifcampaign.com/)

“The goal of the Anti-IF Campaign is to raise awareness of effective use of software design principles and practices, first of all removing bad, dangerous IFs.”

The Good, The Bad and The Ugly

Is it possible to program without IFs? There are many situations where IFs are absolutely required and necessary.

The IFs can be classified into:

  • Good IFs: These consist of algorithmic IFs, conditions essential to an algorithm.

  • Bad IFs: A polymorphic IF, “if is a”, type checking, control coupling

  • Ugly IFs: Null check IFs, I won’t go into these here, but they should also be avoided.

The difficulty sometimes lies in determining whether an IF is good or bad. 

The Exercise

The code sample I chose for my exercise was more or less randomly selected. Sonar findings set me on the path, although the sample didn’t have any serious violations.

The aim of the refactoring exercise was soley to concentrate on the one class without changing the signatures of any public method and it’s exposure to the outside world.

The class chosen for refactoring was “PublishApp”, here the actual JavaDoc:

Application class to save customer data and publish a FsboAd at the end of the SYI/RYI flow.

'Publish' means in the case of a

  • Basis ad: The ad is saved or updated. It's always saved with ACTIVE state.

  • Chargeable ad: The ad is only updated at the end of the RYI flow. The status remains unchanged because chargeable ads get activated during payment process. If PublishApp.publish(SaveParams) is called directly after a successful payment, the ad does NOT get updated again.

Furthermore this app does

  • send confirmation mails for new basis ads, and for new or upgraded chargeable ads.

  • write the changelog entry according to the action performed on the ad.

The Javadoc alone indicates that the code breaks the single responsibility principle. And the Javadoc didn’t even list all the responsibilities.

PublishApp was also responsible for:

  • sending TANs

  • looking up makes / models of vehicles

  • determining whether an ad was new, being updated or being upgraded

The “Main” Method

The main actions of the publish app are:

  • check if a TAN needs to be sent

  • update the customer

  • save or update the ad.

At a quick glance, it appears to be clear and concise. The number of parameters passed to the methods are high.

The fist inline comment is intimidating, ‘needs to be checked ….’ because it reads like a warning, do not move this from here’. It turned out that the comment was misleading. The method doesn’t operate on domain data that changes, and would deliver the same state after a customer or an ad has been updated.  

Strange with the method saveUpdateAd is that the result is passed in and out, therefore the updated customer is passed in twice. Additionally a control boolean is passed in.

The method saveUpdateAd, starts off with defining many control booleans, some final some not. The check whether an ad is a new basic ad, i.e. checking if the ad id is null, is despersed throughout the code. If the definition of a new basic ad changes, it will have to be changed in many places. This is an example of tight coupling between the Ad-Id and the definition of a new Ad.

The next lines in the code reveal the typical arrowhead pattern:

Various states of ads are handled in the IF blocks:

  • basic new ad

  • update basic ad

  • chargeable ad

The handle chargeable ad part becomes a bit more difficult to read with further "complex" control booleans:

A confirmation mail should be sent, if the ad is a new chargeable ad or a package upgrade. This is the exact oposite to the defined boolean ‘updateAd’.

And finally as part of update/insert ad:

An email is sent, the customer is updated, changelog is written and a tan is sent.

Deciding on TAN

Back to the first decision, whether a TAN needs to be sent or not. This was a method in SentTanApp which delegated the decision to FsboTanService.

SendTanApp:

FsboTanService

The following code snippet was duplicated, in both FsboTanApp and PublishApp.

It is very questionable whether the FsboTanApp should be responsible for deciding if a TAN should be sent or not. That the FsboTanApp should decide what state the ad has is certainly a wrong design decision.

Further curiosities in the code:

SentTanApp delegates the decision to FsboTanApp, which is perfectly reasonable. But the exception handling in both methods is misleading. Apart from the fact, that no exception should get through to SendTanApp because they are all caught in FsboTanService, both have a different strategy on handling the exception. Whereas in the one case, a tan is always to be sent when an error occurs, in the other, a tan is never to be sent.

A closer look at the IF blocks to see when a tan should be send:

  • don’t send a TAN if the customer lives outside of Germany

  • only send a TAN if it’s a new ad, either a new basic ad, or a new chageable ad.

The problems with the existing code are:

  • breaks single responsibility principle

  • breaks open/closed principle

  • tight coupling

  • code duplication

  • wrong understanding of separation of concerns

  • mix of control and work to be done

  • obfuscation of flow in extracted methods

Steps to Refactoring

I had no plan on how to refactor the code. My first strategy,  I took Kent Beck’s advice and inlined everything.

The class had a very good test coverage, so I could safely refactor step by step.

The method “isTanSmsRequired” from “FsboTanApp” was inlined and removed it from it’s original location, since it was not used anywhere else and had no state reference to the class.

After inlining everything, the code obviously got a lot more verbose and had more code duplication.

Interesting were the additional conditionals which became apparent for all further actions after an ad was updated or inserted.

Looking at the code it is obvious that PublishApp handles various states of an ad

  • creates a new basic ad

  • updates an existing basic ad

  • updates a chargeable ad

  • does some things with a new chargeable ad

  • does some things with an upgraded ad

and performs actions according to the various states:

  • send confirmation mail if the ad is a new basic ad, a new chargeable ad or an upgraded ad

  • send a tan if the ad is new or a new chargeable ad

  • don’t write a change log action if the ad is a new chargeable ad, or an upgraded ad

Special attention has to be paid to the early return which bypasses all actions and was kind of hidden in a nested structure:

The refactored code

The “main”-Method

The publish still updates the customer and performs actions to the ad, not much has changed. Checking for a tan has been removed, it is no longer required. The horrible exception handling is to handle the early return mentioned above.  The actions to be performed have been relocated to separate classes and are created with a small helper method:

The generation of the actions to be used are deferred to a factory. PublishApp doesn’t handle any ad updates or creation directly any more nor actions like sending mails or sending tans.

The factory provides a seperate method for producing an action for each ad state:

The actions use the decorator pattern and are generated in a fluent style so that it is easy to read which actions are to be performed for which ad state.

Each Action is a relatively small class and has dedicated responsibility, e.g.:

FindAdAction

MailServiceAction

The MailServiceAction decorates another action, of which it doen't know what it does.

All conditionals determining the state of the ad have been moved to those classes that know the most about it’s state, to save params and ad input. The different ad states are defined by an enumeration AdInputType.

With the help of various implementations of AdInputCommands, the factory methods for creating an SyiFsboAdStrategy are invoked. The ad input types are mapped to the commands using a simple mapping.

The commands simply delegate to the corresponding factory methods and are a Java way of holding pointers to functions (the old Java 7 style).

Conclusion

All ‘control’ booleans have been removed from the code. IFs are still present, but these have been moved to where they are of immediate concern. Each ad state is handled by a different class. The repeated actions for various states are achieved by using the decorator patterns.

In the factory for creating the actions, it is easy to read which actions are to be performed for which ad state. The concerns have been separted into many classes. The refactored code can be extended for new features without modifiying the existing code.

In my opinion, it is worth while pondering over every IF and considering whether it is really necessary and how it can be avoided. Practising using design patterns makes design decisions easier. Sometimes it’s quicker to add another IF to existing code, but is not always the best way.  

Links

http://www.antiifcampaign.com/index.html

http://michaelfeathers.typepad.com/michael_feathers_blog/2013/11/unconditional-programming.html

https://coderwall.com/p/wzykgg

http://ewernli.wordpress.com/2010/03/07/anti-if-programming/

http://haacked.com/archive/2013/11/08/death-to-the-if-statement.aspx/

http://programmingwithoutifs.blogspot.de/

http://www.laputan.org/mud/ Big Ball of Mud

java, clean code, design pattern

?>