Code metamorphosis

In this post, I would like to show through an example, how we can always progressively improve even the most badly written piece of code.

I will start from a somewhat unintelligible program and, step by step, rewrite it until it really pleases the eyes. Doing so, I will preserve its external behavior, which is roughly specified as follows:

The program inputs 3 possibly null strings. It shall interpret them as 2 integers and an operator among +, -, * and /, to then evaluate the expression and display the result.
Any invalid input value is signaled by an appropriate error message.

I am assuming the existence of two library methods:

• Integer readInteger(String) transforms a string into the integer it represents. This method expects a non-null argument, and returns null on any string which does not correctly parse as an integer,
• void println(String) prints its string argument on the standard display and terminates with a new line.

Here is our starting point, a program which fulfills the specification:

void calculate(String number1, String operator, String number2) {
  Integer value1;
  Integer value2;
  if (null != number1) {
    value1 = readInteger(number1);
    if (null != value1) {
      value2 = readInteger(number2);
      if (null != value2) {
        if ("+".equals(operator)) {
          println(value1 + value2);
        } else if ("-".equals(operator)) {
          println(value1 - value2);
        } else if ("*".equals(operator)) {
          println(value1 * value2);
        } else if ("/".equals(operator)) {
          if (!new Integer(0).equals(value2)) {
            println(value1 / value2);
          } else {
            println("Division by zero.");
          }
        } else {
          println("Unknown operator: " + operator);
        }
      } else {
        println("Invalid argument: " + number2);
      }
    } else {
      println("First argument is not a valid number.");
    }
  } else {
    println("Missing first argument.");
  }
}

Even though it does the job, this program does not conform my personal aesthetics. Here are the reasons why:

1. Lack of structure. There is a complete absence of separation of concerns in this monolithic, heavily indented piece. This piece does not express its intent at all, which is really to perform 3 distinct steps: validate and parse its input, then perform the computation, and at last present the result (or an error message) to the user.
2. Difficult code extensions. Nowadays, programs often need to change their behavior (sometimes drastically and very quickly). So they must be built in a way that allows for future evolutions. For instance, in our case, what if the result needs to be output in a message box rather than the standard output? Or what if, error messages must be dumped separately on the standard error? Multiple lines of code would need to be rewritten. What about handling more complex expressions (with an arbitrary number of operators)? Or replacing all integers with floating points? Similarly, adding error reporting for integer overflows in an uniform way is not a trivial matter. This code is not cut for change, mainly because of its high level of coupling. Adding more functionality will also add to the current mess and lead to code decay.
3. Hard to read. For such a small task, this code is obviously too hard to understand. It has too many lines of code (length) and a too high level of imbrication (width) for one method. There is no quick way of breaking it down. If kept as it is, maintenance will be costly.
4. Hidden bug. Ignoring the fact that integer overflows are not avoided, there is another bug in this small piece. Did you spot it? Don't worry if you did not: the process of cleaning it up will, at some point, make this bug surface.
5. Low testability. I am having a hard time believing that this piece of code can be thoroughly covered with automatic unit tests. In particular, since it is immediately dumped on the console, I can not think of any straightforward way of sensing the output value of the program. The presentation layer is buried too deeply inside the method body to be readily replaced by a fake implementation suitable for testing purposes. This code needs a serious testability improvement.

Believe it or not, despite its chaotic appearance, this program has its own logic. Let me venture an explanation as to why you may encounter this type of code more frequently than desirable:

1. Single return. Of the epic fight against gotos, remains one bad habit: namely to have only one return point per method. The modern motivation for this rule is to prepare a single break-point for every method when inspecting the program execution using a debugger. Being of the school of thought that running the debugger is already a sign you lost control over your code, I completely disregard this constraint.
2. Error cases come second. Did you notice that else branches always contained the error cases? This is so that the correct execution can be read first.
3. Lazy programming. The coder was driven only by the result to compute. He did not care about readability, maintainability, evolutivity or testability at all. In consequence, inputs are parsed when needed, results are output as soon as they become available in total disregard of any program structure.
4. Low level defensive programming techniques. Were you surprised by the peculiar shape of conditionals? There are two distinct tricks here. First, all calls to equals are made on a constant object, rather than on a variable. (e.g."*".equals(operator)). The rationale being to avoid potential exception to be thrown if the variable happens to be null. On the surface, this may seem reasonable. But I do not like this coding style. Again I believe it shows a lack of control: at every point, the programmer should know whether any variable may be null, or not. If it may then a check should be performed first to handle this case once and for all. If not, then there is no need to worry. It should not be necessary to sacrifice conditional expressions' readability. In any case, later in this post, I will present a programming technique to drastically minimize any null value flowing through the program. Second, in order to ensure a value is not null, this programmer believes that writing (null != x) is preferable to the more straightforward (x != null). This may be true in C, since a small typographical error and the comparison transforms into the assignment x = null. But not so in Java which has a distinct type of boolean. The type-checker protects you.
5. Grouped variable declarations. All variables used in this method are declared together at the beginning of the method. This is a C habit, which is unnecessary in more modern languages such as Java or C#. It is preferable to let variables have the tightest possible scope since it reduces the burden of understanding the meaning of many variables too early. It also decreases risks of misuses. Some would oppose that variables declared inside a loop are inefficient, since they will be allocated and freed multiple times. I honestly don't know if that is true: optimizing compilers most probably know better. In any case, I highly doubt that any performance bottleneck ever arises from such practice.

We are now almost ready to start refactoring this piece of code. Before we do so, I am going to assume there is a non-regression test suite for this method. It is going to be run at the end of each rewriting step in order to make sure the program external behavior is preserved.

 

Step I: Early return and program intent

First, let us insert some early returns. This will enable us to clearly separate the 3 phases of the program: input validation, computation and presentation. For various reasons, some of you may not like early returns. I do. Anyways, as you will see, in this case, they are only a temporary artifact to disappear in a later refactoring phase.

void calculate(String number1, String operator, String number2) {
  // Phase 1/3: validate input 
  if (number1 == null) {
    println("Missing first argument.");
    return;
  }
  Integer value1 = readInteger(number1);
  if (value1 == null) {
    println("First number is not a valid integer.");
    return; 
  }
  Integer value2 = readInteger(number2);
  if (value2 == null) {
    println("Invalid integer: " + number2);
    return;
  }
  if (operator == null) {
    println("Missing operator.");
    return;
  }
  // Phase 2/3: computation
  Integer result;
  switch (operator) {
    case "+":
      result = value1 + value2;
      break;
    case "-":
      result = value1 - value2;
      break;
    case "*":
      result = value1 * value2;
      break;
    case "/":
      if (value2.equals(0)) {
        println("Division by zero."); 
        return;
      }
      result = value1 / value2;
      break;
    default:
      println("Unknown operator: " + operator);
      return;
  }
  // Phase 3/3: presentation
  println(result);
}

Here is a list of things we did in this step :

• Early returns were introduced in order to eliminate error cases as soon as possible. They allowed us to separate the input validation from the rest of the method.
• An intermediary variable was introduced to store the computation result and separate the presentation layer from the rest of the method.
• Variable declarations were delayed until needed. Variable initialization is now extremely close from, if not in the same statement as, its declaration.
• Conditionals were inverted to feel more natural, and the sequence of if/else-if was replaced by a switch statement to make the alternatives more visual.

Admittedly, the program is now longer, but a lot easier to understand. However, in doing so, we also slightly changed the behavior of the program: in the case where the operator is null, the error message became "Missing operator." instead of the previous "Unknown operator: null".

 

Step II: Extract methods and expressions

Let us stop for a moment, and take some time to listen to our code. As I am gaining experience, I am finding this is a worthy practice. Often, the code knows better what it wants to look like. We just have to take notice, handle it softly and follow its lead. Here, the two first large blocks, each starting with a line of comment are eager to become full-fledged methods. In order to increase readability, we will thus extract two methods:

• Expression parseInput(String number1, String operation, String number2)
• Integer evaluate(Expression expression)

To do so, we first need to introduce the Expression: an object which encapsulates the output of the parsing phase and which is then evaluated. The expression consists in two integers combined with an operation:

enum EOperation { Plus, Minus, Mult, Div }

class Expression {
  private Integer value1;
  private EOperation operation;
  private Integer value2;
  Expression(Integer value1, EOperation operation, Integer value2) {
    this.value1 = value1;
    this.operation = operation;
    this.value2 = value2;
  }
  public Integer getValue1() { return this.value1; }
  public EOperation getOperation() { return this.operation; }
  public Integer getValue2() { return this.value2; }
}

The code can now easily be rewritten into :

void calculate(String number1, String operator, String number2) {
  Expression expression = parseInput(number1, operator, number2);
  if (expression == null) return;
  Integer result = evaluate(expression);
  if (result == null) return;
  println(result); 
}

Expression parseInput(String number1, String operator, 
                      String number2) {
  if (number1 == null) {
    println("Missing first argument.");
    return null;
  }
  Integer value1 = readInteger(number1);
  if (value1 == null) {
    println("First number is not a valid integer.");
    return null;
  }
  Integer value2 = readInteger(number2);
  if (value2 == null) {
    println("Invalid integer: " + number2);
    return null;
  }
  if (operator == null) {
    println("Missing operator.");
    return null;
  }
  EOperation operation;
  switch (operator) {
    case "+":
      operation = EOperation.Plus;
      break;
    case "-":
      operation = EOperation.Minus;
      break;
    case "*":
      operation = EOperation.Mult;
      break;
    case "/":
      operation = EOperation.Div;
      break;
    default:
      println("Unknown operator: " + operator);
      return null;
  }
  return new Expression(value1, operation, value2);
}

Integer evaluate(Expression expression) {
  Integer result;
  switch (expression.getOperation()) {
    case EOperation.Plus:
      return expression.getValue1() + expression.getValue2();
    case EOperation.Minus:
      return expression.getValue1() - expression.getValue2();
    case EOperation.Mult:
      return expression.getValue1() * expression.getValue2();
    case EOperation.Mult:
      if (expression.getValue2().equals(0)) {
        println("Division by zero.");
        return null;
      }
      return expression.getValue1() / expression.getValue2();
  }
  throw new IllegalArgumentException();
}

There are admittedly now more methods and the code looks even more verbose. But the gain in structure and modularity should be well worth it.
You may be wondering the need to throw an IllegalArgumentException at the end of method evaluate. That is because the compiler does not know this line of code is unreachable (because all enum cases are covered in the switch). It would therefore otherwise emit a compilation error.
Note also, that we did not include any default case in the preceding switch statement. This is done on purpose, so that, if we are to augment the enum with another operation, we will be warned at compile time of all the cases which are not handled.

 

Step III: Exceptions and clean error report

There are two strong code smells left in the previous piece of code:

• methods parseInput and evaluate both propagate errors up by encoding them into the null return value,
• error messages are scattered all around.

There is therefore still a high level of coupling between the code which performs the computation task, error handling and the presentation layer. We are going to remedy this situation with the introduction of two exceptions which will respectively signal parse errors and computation errors :

class ParseError extends Exception { }
class DivisionByZero extends RuntimeException { }

void calculate(String number1, String operator, String number2) {
  try {
    Expression expression = parseInput(number1, operator, number2);
    Integer result = evaluate(expression);
    println(result);
  } catch (ParseError e) {
    println("An error occurred while parsing: " + e);
  } catch (DivisionByZero e) {
    println("Division by zero");
  }
}

Expression parseInput(String number1, String operator, 
                      String number2) throws ParseError {
  if (number1 == null) {
    throw new ParseError("Missing first argument.");
  }
  Integer value1 = readInteger(number1);
  if (value1 == null) {
    throw new ParseError("First number is not a valid integer.");
  }
  Integer value2 = readInteger(number2);
  if (value2 == null) {
    throw new ParseError("Invalid integer: " + number2);
  }
  if (operator == null) {
    throw new ParseError("Missing operator.");
  }
  EOperation operation;
  switch (operator) {
    case "+": 
      operation = EOperation.Plus;
      break;
    case "-":
      operation = EOperation.Minus;
      break;
    case "*":
      operation = EOperation.Mult;
      break;
    case "/":
      operation = EOperation.Div;
      break;
    default:
      throw new ParseError("Unknown operator: " + operator);
  }
  return new Expression(value1, operation, value2);
}

Integer evaluate(Expression expression) {
  Integer result;
  switch (expression.getOperation()) {
    case EOperation.Plus:
      return expression.getValue1() + expression.getValue2();
    case EOperation.Minus:
      return expression.getValue1() - expression.getValue2();
    case EOperation.Mult:
      return expression.getValue1() * expression.getValue2();
    case EOperation.Mult: 
      if (expression.getValue2().equals(0)) {
        throw new DivisionByZero();
      }
      return expression.getValue1() / expression.getValue2();
  }
  throw new IllegalArgumentException();
}

I am now fairly satisfied with the current state of the code. We still can and will go a bit further. However, before doing so, we are first going to eliminate a bug and improve test coverage.

Step IV: Fixing a bug and DRY investment

By now, it should be obvious! The parsing phase has a flaw in that it does not check whether the second number may be null. Let us first write a non-regression test which fails:

public void 
test_parseInput_ShouldThrowParseErrorWhenSecondNumberIsNull() {
  try {
    Program program = new Program();
    program.parseInput("0", "+", null);
    Assert.fail();
  } catch (ParseError e) {
  }
}

Please take note how straightforward that test was to write. Writing a similar test harness with the first, monolithic, version of the code, would have been a lot less fluid. It would have required us to first redirect the standard output into a buffer, in order to then assert the correct error message is printed out. Learning how to write such complicated tests is a good exercise. Especially so if you are working with legacy code. But in the long run, it is not worth it. It is largely preferable to rewrite your code in order to make it easily testable.
Fixing the bug would be equally easy. But we are going to make a bit more work than necessary and take this opportunity to further clean up the code. Faithful to the DRY (Don't Repeat Yourself) motto, we will eliminate any repetition by introducing three more methods :

• ensureIsNotNull(String input) throws a ParseError on a null input,
• parseInteger(String input) converts the string representation of an integer into its value, and throws a ParseError to signal failure,
• parseOperation(String input) converts the string representation of an operation into a EOperation.

Here are the methods whose code is modified:

Expression parseInput(String number1, String operator, 
                      String number2) throws ParseError {
  Integer value1 = parseInteger(number1);
  EOperation operation = parseOperation(operator);
  Integer value2 = parseInteger(number2);
  return new Expression(value1, operation, value2);
}

void ensureIsNotNull(String input) throws ParseError {
  if (input == null) throw new ParseError("Missing input.");
}

Integer parseInteger(String input) throws ParseError {
  ensureIsNotNull(input);
  Integer result = readInteger(input);
  if (result == null) throw new ParseError("Integer expected.");
  return result;
}

EOperation parseOperation(String input) throws ParseError {
  ensureIsNotNull(operator);
  switch (input) {
    case "+": return EOperation.Plus;
    case "-": return EOperation.Minus;
    case "*": return EOperation.Mult;
    case "/": return EOperation.Div;
    default:
      throw new ParseError("Unknown operator: " + operator);
  }
}

I would like to draw your attention to the fact that it is extremely easy (in comparison to a large if else block) to move an unitary instruction such as ensureIsNotNull(number1); around the code. We could for instance, push it up, in an effort to eradicate null values as early as possible. Removing all null values at system boundaries, allows us to safely assume no value can ever be null in the code body. Reducing of that much the risk of null pointer exceptions.

Even more importantly, at the same time we fixed the bug, we limited even more the probability of a similar bug to reappear later. Here, the safeguard consists in having method ensureIsNotNull being called inside method parseInteger. So that, as long as we use this method to parse any other integer, then the same mistake can simply not, by construction, be made.

Step V: Improving testability and seam

Unit tests that assert the correctness of error messages would also be nice to have. These messages may, for instance, be contractual.
One way to do so, would be to redirect the standard output into a buffer (using method System.setOut in Java). Another, simpler way, would be to plug a fake output stream inside the program constructor and write the test as follows:

public void test_calculate_SignalsDivisionByZero() {
  FakeOutpuStream output = new FakeOutputStream();
  Program program = new Program(output);
  program.calculate("10", "/", "0");
  Assert.assertEquals("Division by zero", 
                      output.getLastPrintedLine());
}

There are two necessary ingredients to make such a test possible. First, a FakeOutputStream, that is a custom OutputStream which really does nothing except store the last printed string. Method getLastPrintedLine can then be called to retrieve this string in order to build the assertion. Second and more importantly, it should be possible to choose the output stream used for all outputs of the program. The best way is to have the program constructor accept this dependency as a parameter. After the previous refactoring steps, methods println is now called in very few different places and it is thus very easy to modify the program to this end:

class Program {
  private OutputStream output;

  Program(OuputStream output) {
    this.output = output;
  }

  void calculate(String number1, String operator, String number2) {
    try {
      Expression expression = parseInput(number1, operator, number2);
      Integer result = evaluate(expression);
      this.output.println(result);
    } catch (ParseError e) {
      this.output.println("An error occured while parsing: " + e);
    } catch (DivisionByZero e) {
      this.output.println("Division by zero");
    }
  }

  // ...
}

As often, by making the program more testable, we also improved its design. The fact that this piece of code is printing to a stream can now be deduced looking at the constructor signature only. It also becomes easy to plug different kinds of presentation layer, as long as they extend the OutputStream interface.
To inject the output stream program dependency may have seem fairly easy to do here. Sometimes, it may require more work or may not appear so evidently. In any case, it is a very powerful and sane programming technique. This technique supports the stance that we are not building monolithic programs but rather program components. A major coding attitude shift.
As a final note for this section, let me admit that error messages should ideally not be present as constant strings in the program but rather all read from an external resource. But that is for another story.

Step VI: Evaluation the OO way

Let us now wander into the realm of (hard-core) object orientation, and for some of you maybe superfluous bonuses. This next rewrite step originates from a simple observation:

Method evaluate does not need the instance of Program at all, but it rather extensively accesses fields of its unique argument, the Expression.

Consequently the program would surely gain in encapsulation and modularity, if we were to move this method into class Expression. Going a step further, it would make even more sense to dispatch the treatment for each operator into the operations themselves which would relieve the need for the switch statement. It is actually common knowledge that switch statements rigidify object oriented programs. The conventional remedy is a small amount of object inheritance. Here is how it can be done in practice:

interface IOperation {
  Integer evaluate(Integer value1, Integer value2); 
}

class PlusOperation implements IOperation {
  public Integer evaluate(Integer value1, Integer value2) {
    return value1 + value2;
  }
}

class MinusOperation implements IOperation {
  public Integer evaluate(Integer value1, Integer value2) {
    return value1 - value2;
  }
}

class MultOperation implements IOperation {
  public Integer evaluate(Integer value1, Integer value2) {
    return value1 * value2;
  }
}

class DivOperation implements IOperation {
  public Integer evaluate(Integer value1, Integer value2) {
    if (value2.equals(0)) throw new DivisionByZero();
    return value1 / value2;
  }
}

class EOperation {
  public static final PLUS = new PlusOperation();
  public static final MINUS = new MinusOperation();
  public static final MULT = new MultOperation();
  public static final DIV = new DivOperation();
}

class Expression {
  private Integer value1;
  private IOperation operation;
  private Integer value2;

  Expression(Integer value1, IOperation operation, Integer value2) {
    this.value1 = value1;
    this.operation = operation;
    this.value2 = value2;
  }

  Integer evaluate() {
    return this.operation.evaluate(value1, value2); 
  }
}

class Program {
  // ...
  void calculate(String number1, String operator, String number2) {
    try {
      Expression expression = parseInput(number1, operator, number2);
      Integer result = expression.evaluate();
      this.output.println(result);
    } catch (ParseError e) {
      this.output.println("An error occurred while parsing: " + e);
    } catch (DivisionByZero e) {
      this.output.println("Division by zero");
    }
  }
  // ...
} 

Each operation has now its own class. These classes all inherit from a common interface which declares an evaluate(String, String) method. To ensure some sort of backward compatibility, we kept an EOperation class, which can be used as an enum by providing several static, final, non-modifiable singleton operations. To be honest, I would never use this trick in an industrial setting. (And don't worry, I will get rid of it in the final refactoring step). For all the reasons exposed in my previous post (see A case against static), this is borderline and unnecessary. But I wanted to make the point that the parsing part of the code need not be changed at all in this refactoring phase (plus, for once, I wanted to please all the memory performance junkies out there: doing so avoids a re-allocation of an IOperation at every call to method calculate).

Without any doubt, this rewrite steps improved the code structure. Note how none of the fields in class Expression escapes the private scope anymore. A sure sign of better encapsulation. Furthermore, modification of the code to handle another operation would be largely more localized than in the initial piece.

Step VII: Ultimate polishing step

Before feeling satisfied, we are going to eliminate the last code smells: the presence of a switch statement and a (fake) enum. To do so I will use yet another trick: that of changing control flow code into data. We will simply put all operations into a hash-map indexed by the string representation of the operation. Then parsing any operator simply amounts to getting the corresponding operation into the hash-map. If no operation is found, then it is a parsing error. As a bonus, since the hash-map will be initialized once in the program constructor, there is really no need for the fake EOperation enum anymore. Here are the two methods which change during this rewrite step:

class Program {
  private OutputStream output;
  private HashMap<IOperation> operations;

  Program(OuputStream output) {
    this.output = output;
    this.operations = new HashMap<IOperation>();
    this.operations.put("+", new PlusOperation());
    this.operations.put("-", new MinusOperation());
    this.operations.put("*", new MultOperation());
    this.operations.put("/", new DivOperation());
  }
  // ...
  IOperation parseOperation(String input) throws ParseError {
    ensureIsNotNull(operator);
    IOperation result = this.operations.get(operator);
    if (result == null) 
      throw new ParseError("Unknown operator: " + operator);
    return result;
  }
  // ...
}

 

A word on the final code version

Let us have a look at the final version of the code:

interface IOperation { 
  Integer evaluate(Integer value1, Integer value2); 
}

class PlusOperation implements IOperation {
  public Integer evaluate(Integer value1, Integer value2) {
    return value1 + value2;
  }
}

class MinusOperation implements IOperation {
  public Integer evaluate(Integer value1, Integer value2) {
    return value1 - value2;
  }
}

class MultOperation implements IOperation {
  public Integer evaluate(Integer value1, Integer value2) {
    return value1 * value2;
  }
}

class DivOperation implements IOperation {
  public Integer evaluate(Integer value1, Integer value2) {
    if (value2.equals(0)) throw new DivisionByZero();
    return value1 / value2;
  }
}

class Expression {
  private Integer value1;
  private IOperation operation;
  private Integer value2;

  Expression(Integer value1, IOperation operation, Integer value2) {
    this.value1 = value1;
    this.operation = operation;
    this.value2 = value2;
  }

  Integer evaluate() {
    return this.operation.evaluate(value1, value2);
  }
}

class Program {
  private OutputStream output;
  private HashMap<IOperation> operations;

  Program(OuputStream output) {
    this.output = output;
    this.operations = new HashMap<IOperation>();
    this.operations.put("+", new PlusOperation());
    this.operations.put("-", new MinusOperation());
    this.operations.put("*", new MultOperation());
    this.operations.put("/", new DivOperation());
  }

  void calculate(String number1, String operator, String number2) {
    try {
      Expression expression = parseInput(number1, operator, number2);
      Integer result = expression.evaluate();
      this.output.println(result);
    } catch (ParseError e) {
      this.output.println("An error occured while parsing: " + e);
    } catch (DivisionByZero e) {
      this.output.println("Division by zero");
    }
  }

  Expression parseInput(String number1, String operator, 
                        String number2) throws ParseError {
    Integer value1 = parseInteger(number1);
    IOperation operation = parseOperation(operator);
    Integer value2 = parseInteger(number2);
    return new Expression(value1, operation, value2);
  }

  void ensureIsNotNull(String input) throws ParseError {
    if (input == null) throw new ParseError("Missing input.");
  }

  Integer parseInteger(String input) throws ParseError {
    ensureIsNotNull(input);
    Integer result = readInteger(input);
    if (result == null) throw new ParseError("Integer expected.");
    return result;
  }

  IOperation parseOperation(String input) throws ParseError {
    ensureIsNotNull(operator);
    IOperation result = this.operations.get(operator);
    if (result == null) {
      throw new ParseError("Unknown operator: " + operator);
    }
    return result;
  }
}

First, if you examine conditionals, you will see they are all of the most simple form: if some condition, then throw some exception. Two of them perform input validation, the other two filter results returned by calls to library functions (parseInteger and get from the HashMap can both return null). To sum up, conditionals are all placed at the system boundaries to eliminate unwanted values. It is no coincidence, but rather a choice to perform defense at the perimeter and to fail brutally the earliest possible.
Collecting a few metrics, we can produce the following tables:

Structure	Classes	Methods	Interfaces	Hierarchy depth
First program	1	1	0	0
Last program	6	12	1	1

Complexity	Instructions	Max. instructions/function	Max. imbrication level
First program	26	26	5
Last program	44	7	1

Control flow	if	else	switch	throw	catch
First program	8	8	0	0	0
Last program	4	0	0	3	2

Features	occurrences of null	enum	generic
First program	3	0	0
Last program	3	0	1

Even though this program has more code than the initial version, methods are all very short. The longest one (method evaluate) has only 7 instructions. And the maximum imbrication depth is only one. Taken in isolation, each method does not amount to much, but it is the combination which proves valuable. Testing multiple small methods is easy. So much so, that some developers will not understand the value of such simple tests: seeing it only as an unnecessary repetition of what is already obvious in the code itself. Adding new behavior will be faster and more localized than in the initial monolithic version.
In brief, that is how I like software: minimalist.

Conclusion

In conclusion, I would like programmers to not satisfy themselves with code written in the style of the first sample of this post. Aesthetic matters.
At another level, I would like managers to understand that code quality is desirable, that change is possible. Not only that, but semantics preserving micro-changes are possible too. We performed a drastic code rewrite a step at a time. Each step was fairly small, allowing small commits and minimizing risks. Success is a journey, not a starting point.

Refactoring code is a multi-facet activity. Let me recapitulate the various techniques that were introduced in this post:

• separating concerns (step I),
• inverting conditionals and using early returns to make the code more malleable (step I),
• slightly altering the program behavior (sometimes even the specification) to open up more factorization opportunities (step I),
• detecting code smells (step II),
• replacing comments by well-named methods (step II), 
• extracting methods to improve structure (step II),
• using exceptions to reduce cognitive load and fail early (step III),
• cleaning up code to let bugs surface (step IV),
• adding test before fixing bug (step IV),
• DRY (step IV),
• taking advantage of fixing bugs to clean up code (step IV),
• making code testable (step IV and V),
• injecting dependencies as constructor parameters to enable tests (step V),
• replacing switch statements with inheritance (step VI),
• increasing object encapsulation by moving methods around (step VI),
• converting code into data: switch into hash-maps (step VII),
• eliminating null from the system, filtering it at the boundaries, having defense at the perimeter (final word).

A case against static

Introduction

Some people believe that all constructs of a programming language have their raison d'être. I am of the opposite school of thought:  I strive hard to write code using extremely reduced, simple and coherent subsets of the language. Because I know this practice is a major factor of code quality. If I was given the choice to remove only one keyword from the Java programming language, I would have no hesitation whatsoever. It would be the static keyword.

I will not dwell into a detailed explanation of the meaning of keyword static. Only that it comes in four flavours: static variables, static methods, static classes and static initialization blocks. Static variables are associated to the class in which they are declared (contrast this to standard fields which necessarily appear in an instance of the class). They can be roughly understood as global variables (with various degrees of visibility). Similarly static methods do not need any class instance to be called, but can not access non-static fields. Static (necessarily inner) classes are somewhat a different story which I prefer not to tell today. (In short, I despise inner classes and ban them in my coding standards). And at last, static initialization blocks are not really worth mentioning: they are simply bug nests to avoid at all costs.

First let me stress the fact that, from a theoretical point of view at least, any program can be written with no static variables nor methods at all (except for the entry point, which is by definition necessarily static). The proof is easy to sketch: group all static variables and methods as non-static members of one huge class. Initialize one instance of this class at the beginning of the program and then pass this instance all over the place for the other classes to use. 

Conversely, note that a Java program with only static variables and methods looks like a C program with modules, but without pointers, structures or unions. Pointers to structures can be somewhat emulated by classes without any methods. Hardcore C programmers must now be grinding their teeth: I am well aware Java does not allow for the power of low level operations such as pointer arithmetic or arbitrary casts (for example from structure to array of characters). Unions however can be recovered with some clever tricks using inheritance.

This brief language analysis underlines the fact that really two orthogonal ways of organizing code are competing within Java:

• pure modular procedural programming (only static methods, and non-static variables in lightweight data objects),
• pure dependency-injected object orientation (no static variables or methods).

In practice, most programs I have encountered are a mix of these two relatively incompatible paradigms. And it feels. So, I am going to make the case, that writing within the pure object oriented fragment of Java is more productive and leads to more robust code.

Problem statement

Let me start by examining some very small code snippets. Although simple, these examples are representative of the use of static methods in real-world programs.

The most extensive use of static methods is to make a service accessible from any place in the code:

Service.perform();

On the surface, nothing wrong here. However, the truth is that real programs are rarely that simple. In most concrete cases, simply calling a static method such as perform will simply not work. The method will throw some exception, because some internal structure needs to be initialized first. To do so, one needs to call yet another static method first:

Service.initialize();
Service.perform();

This is not the end of the story yet. Method initialize will usually be called once during the setup phase of the application (or maybe worse from another class's initialize method), whereas perform gets called several times, wherever it is required. The two methods, although logically bound, are thus syntactically far apart in the code:

// initialization phase
Service.initialize();
...
// application body
Service.perform();

You may think it is not a big deal. In fact, browsing the documentation of class Service may be sufficient to quickly understand its correct usage. (Even though documentation has an awkward tendency to age very quickly).

But, let us now go just one step further. Consider this piece of code:

Treatment treatment = new Treatment();
treatment.process();

There is a trap in this code. It is hidden from the eyes of the unaware external reader. What matters here, is that somewhere down in the body of method process, lies a call to the same static method Service.perform. This means that method process can be used only under the condition that Service is correctly initialized.

Service.initialize();
...
Treatment treatment = new Treatment();
treatment.process();

Now multiply this pattern many fold, spread it all over your application and you just got yourself a maintenance nightmare. Herein lies the core of the problem with using static: the introduction of hidden dependencies, also called hidden temporal coupling. So called temporal because Service.initialize must be called before and hidden because no indication is visible from the signature of either the constructor of object Treatment or its method process. As a side-note, the book Clean Code, A Handbook of Agile Software Craftsmanship lists hidden temporal coupling among its code smells.

Medusa by Gian Lorenzo Bernini, 1630
Do not let it petrify your code base!

 

Solution

Now contrast the previous code with the pure object oriented alternative. Instead of calling two static methods, one must first create an instance of the Service. Then method perform can be later called on this instance:

Service serviceProvider = new Service();
...
serviceProvider.perform();

The instance of Service constitutes a tangible proof that initialization correctly took place and a guarantee that method perform may be safely called.

Of course, now, the programmer must do the effort to propagate this instance everywhere he wants to use the service. For instance, the serviceProvider could be passed as a parameter of the Treatment constructor:

Treatment treatment = new Treatment(serviceProvider);
treatment.process();

The dependency is made clear by the signature of constructor Treatment. At first, this coding style may, for some, seem more demanding. However it is really more relaxing. There are no surprises. The programmer can safely rely on the signatures of the constructors and methods to ensure and document all dependencies.

Now that I presented you with the problem and its solution, let me list the multiple reasons why static and in particular hidden temporal coupling is rather bad for your health.

Perseus slaying Medusa
by Laurent-Honoré Marqueste, 1876

The several evils of static

Steep learning curve

Of all the problems related to static, this may seem, in theory, the least worrisome. However, in practice, it will probably consume developpers' time the most. Whenever a coder needs to work on a piece he did not write in the first place, he will have a hard time discovering the implicit dependencies. Unless code is very well documented (and documentation is kept up to date), he will probably need to ask someone more experienced which static methods must be magically called. He will not be able to rely on automatic completion either, since static methods are not carried by the data they operate on.

Costly component reuse

Calling static methods makes code reuse very costly. But that becomes noticeable only at the last minute, when actually trying to extract a piece of code and incorporating it elsewhere. You will have a hard time bringing the result to compile and execute correctly. Only then will it become clear, how heavily the displaced piece of code relied on hidden dependencies. Every single static call must be painstakingly tracked and replaced by some alternative provided in the new execution context. Another solution may seem faster to implement but is even less acceptable: add all the dependencies into the new context. To state it bluntly, highly coupled code can not be cheaply reused.

Increased risks of memory leaks

The deallocation of static variables (and everything that may transitively hold onto them) has to be explicitly taken care of. Programmers familiar with languages having garbage collectors (myself included), tend to easily forget this issue. In our defense, too many details must be handled:

• to write a method which releases memory,
• not to forget calling it everywhere it is needed.

On the opposite, the memory graph held by non-static variables simply frees when program execution leaves the scope. If the documentation is not accurate, programmers may be totally unaware that a static method allocates permanent memory. Which can easily lead to memory leaks. In this case again, the API lies!

Testability issues

Unless you still apply software development techniques from the previous century, you write unit tests. Static methods and variables make unit tests a whole harder to write:

• dependencies must be discovered in order to correctly set the initial state,
• memory not released in a test may impact the next one.

Worst of all, static methods do not provide seams. To perform unit tests, it is often the case that dependencies must be replaced by fake implementations. For instance, suppose you are testing a piece of code which emits orders to a printer:

Component c = new Component();
c.process();

Suppose then that method process performs the printing order with the static call sendPage:

void process() {
    Result result = this.doSomething();
    Page pageToPrint = this.presentResult(result);
    Printer.sendPage(pageToPrint);
}

You would rather not empty another printer ink cartridge every time the test suite is executed. However, there is no easy way to change the behavior of method sendPage for the duration of the test only. One way, which I clearly do not recommend, would be to add yet another static method setImplementation to class Printer. Then the test would go like this:

FakePrinter fakePrinter = new FakePrinter();
Printer.setImplementation(fakePrinter);
Component c = new Component();
c.process();

The much more straightforward solution is to have the constructor for Component depend explicitly on an interface of a printer, which may either be the real Printer (in production code) or a fake (in test code).

FakePrinter fakePrinter = new FakePrinter();
Component c = new Component(fakePrinter);
c.process();

Other examples in the same vein could include a logger whose state you would like to check, the queue of a thread runner which you would like not to fill, files which you would rather not create...

Some hard to track bugs

Static methods and variables are source of bugs of the hard kind. Let me simply illustrate with a real case I once stumbled upon. The application had a configuration service implemented with static methods. In order to retrieve the string value of a property, one would call:

String Configuration.getValue(String key);

The service also had an initialization method:

void Configuration.initialize(InputStream file);

Initialization would read all the configuration key-value pairs present in the input stream and fill a hash table. Calling getValue after initialization would return the property configured by the user. However, calling getValue before would return some default value (most of the time adequate but possibly different from the user's wish). Obviously method Configuration.getValue was called all over the place, even in the program initialization phase. So after some code refactoring, I had unknowingly moved a call to getValue method before the initialization phase. This bug was found very late because no regression test was done on this particular value, and everything seemed to work fine with the default value. It also took some time to pinpoint the root cause of the problem.

Without static, this problem can simply not arise:

Configuration configuration = new Configuration(InputStream file);
...
configuration.getValue("some-key");

Simply because one must first hold an instance of Configuration in order to be able to read some configuration value. And the configuration file is necessarily read when calling the constructor. This category of bugs is a classical consequence of hidden temporal coupling.

Similarly, memory leaks may also cause costly bugs, found late in the development cycle.

Architecture erosion

Static methods and variables are by their very nature global: they can be easily accessed from anywhere in the application. Pressed by time, developers may be tempted to use these handy static methods without paying their true cost upfront: carefully thinking about the overall architectural logic. Doing so, they introduce additional, hidden, dependencies. The application architecture quickly decays.

Unnecessary coupling

What you don't see, doesn't bother you... until it hurts you. Hidden coupling is bad, because it is hidden. So you won't spend time auditing dependencies and cleaning them up. With time, unnecessary coupling will undoubtedly increase without you even taking notice. So removing static methods should be a top priority. It will take time. You will discover unexpected, sometimes frightening links in your application. But at least, once dependencies are explicit, you can work on them: move them around, remove some, divide others... In the end you will get minimally coupled tight and focused pieces of code.

Static propagates static

At last, I am under the impression that static leads to more static. This may be caused by the fact that static methods can not call non-static methods or access instance variables. So when a developer needs to extend the behavior of a static method, he may feel stuck. Instead of trying to remove the static method, he may choose the path of least resistance by simply adding more static methods or fields. He will then gradually encounter more difficulties writing truly object-oriented code. For instance inheritance will not be possible. He will make more data public, losing encapsulation. He will write more code like this:

A.fill(b);
A.process(b);
A.print(b);

At this point, he ends up being trapped in a C style of programming where objects are used as passive data-structures.

Acceptable uses of static

For the sake of balance, there are, in theory, some harmless uses of static. They all obey two conditions:

• no hidden temporal coupling,
• no global mutable state. 

In other word stateless. Let me list all the examples that I can think of.

Constants

When final, static variables are acceptable. Strings, integers fall under this category. However, non literal final data-structures (such as hash tables) are not, since their content varies throughout the life of the application. Hand-crafted enums, implemented as several constant objects are also valid. Loggers may be admissible, even though static loggers become a problem as soon as you wish to mock them for testing purposes.

Fresh results

Static methods which return a new result every time are benign. They often provide alternative constructors. For instance the Matrix class may have a default constructor with only zeros as well as a static method identity to build a matrix with its diagonal filled with ones. Careful though, because singletons with global mutable state are only one step away. So, in this particular case I would rather have an instance of a MatrixFactory with a non-static buildIdentityMatrix method.

By the way, about singletons, Misko Hevery wrote a very well-thought and exhaustive piece underlining their dangers here.

Pure methods

Static methods which work only on the state carried by their arguments are also in theory non-lethal. A nice concrete example being the several assertion methods provided by the Junit framework (say for instance Assert.assertEquals). However, most of the time, having such methods is a sign of bad design. The method should be carried by the object it modifies. The only acceptable exception could be final (sealed in C#) objects, whose behavior can not be modified by inheritance.

But even in this case, I would either build a manager or encapsulate rather than add static methods. In C#, there is also always the solution of extension methods. But are they a good thing? For lack of experience with this language construct, I haven't made up my mind yet.

Program entry point

Whether you like it or not, you can't escape the fact that the program entry point in either C# or Java is a static method!

Head of Medusa
by Peter Paul Rubens, 1617
Let her rest in peace!

Conclusion

For the notable exception of the program entry point and literal constants, all uses of the static keyword should be banned. At first static methods and variables may seem convenient; especially for lazy programmers. But the cost is simply too high: hidden temporal coupling will rot away your program. The consequences range from a steep learning curve, decreased reusability, poor testability to rigid design. The alternative is to explicitly trace dependencies with object instances which are propagated through the constructors or methods parameters. In return, the signature of each class naturally documents all its dependencies.

Automatic tests and system library: a short riddle

Today, let me offer a slightly different kind of post. For once, I will not give any advice but rather propose a riddle for you to solve.

As I have already mentioned in a previous post, regularly running automatic test suites (à la JUnit) is a guarantee against program decay. With a continuous integration server such as Jenkins, reports periodically built from test results present an up-to-date status of the code condition. Test suites are a barrier against regression. Thus enabling, sometimes extremely aggressive, code refactoring. Some teams even use automatic tests as a non perishable form of documentation, or alternatively as executable specifications. The cost of the manual verification phase can also be reduced, by progressively converting the most repetitive testing scenarios into automatic tests. In short, automatic tests let you attain a surprisingly high level of software quality.

So, it is considered good practice, to at least add one automatic test, for every bug found and fixed. To adopt test driven development may even be more rewarding, but that is another story. However, making a manual verification scenario totally automatic may sometimes prove particularly tricky. This is, in particular, the case for any piece of software which depends on some low level library upon which the programmer has no control whatsoever. Herein lies the crux of today's riddle: how to write automatic tests for code relying on non modifiable external libraries.

Let us consider a concrete Java example:

public class Program {
    public static void main(String[] arguments) throws Exception {
        if (arguments.length < 1) return;
        Program program = new Program();
        program.process(arguments[0]);
    }

    public void process(String fileName) throws Exception {
        ...
        FileInputStream file = new FileInputStream(fileName);
        ...
        int value = file.read();
        ...
        file.close();
    }
}

During its execution (method process), this program opens the file (new FileInputStream(fileName)) whose name is passed as argument to the entry point (method main). It then reads some content from this file (int value = file.read()).

Let us suppose the validation team found the following problem: during one of its run, the program brutally stopped. The call to method read unexpectedly threw a java.io.IOException. This may happen, for instance, when accessing a distant file system which suddenly becomes unavailable because of a severed cable. This is clearly an execution context which is difficult to systematically reproduce at each run of an automatic test.

How would you write an automatic non-regression test which replays this scenario?

Oedipus and the Sphinx

by Ingres, 1808

by Gustave Moreau, 1864

by Salvador Dali, 1960

On exceptional virtues

Some time ago, a friend of mine let me know about this post by Joel on software. I usually tend to agree with Joel's opinion. But I couldn't align with this piece which is strongly in favor of returning error codes rather than throwing exceptions. So I am going to propose the counter-point and explain why I'd rather use exceptions.

First, let me get this straight: even though I happen to know C quite well, on the contrary C++ is a foreign and feared programming language to me. So please keep in mind that what I have to say applies mostly to languages with garbage collection such as C# or Java. Languages must be understood as a whole and some traits can be incompatible. And so it goes with exceptions too.

I believe programs which use error codes tend to be more verbose and intricate. Consider these two programs:

try
{
    input = read();
    result = process(input);
    output(result)
}
catch (SomeException)
{
    report_error();
}

and

input = read();
if (input == null)
{
    report_error();
    return;
}
result = process(input);
if (result == null)
{
    report_error();
    return;
}
if (!output(result))
{
    report_error();
}

So which one would you rather have?
It can even get worse as half of the population of developers seem to favor functions with a single-exit point. The previous example then becomes:

input = read();
if (input != null)
{
    result = process(input);
    if (result != null)
    {
        if (!output(result))
        {
            report_error();
        }
    }
    else
    {
        report_error();
    }
}
else
{
    report_error();
}

We just made a 10 line function span over 20 lines. So is my brain under-performing? Or does using error codes increase the cognitive load? One thing is for certain. Avoiding exceptions forces you to violate two important programming principles:

• Don't Repeat Yourself: because there are duplicate versions of the error handling code.
• Single Responsibility Principle: since the logic of the program is interwoven with the management of exceptional cases.

Some C programmers overcome their inbred aversion for gotos in order to mimic exceptions and write code such as this:

  input = read();
  if (input == null) goto error;
  result = process(input);
  if (result == null) goto error;
  if (output(result)) goto end;
error:
  report_error();
end:
  return;

These programmers surely seem to agree with me.

Let us now suppose for a moment we still go for the lengthy version. After all, having more lines of code makes our productivity statistics look good, doesn't it? Now, we are faced with another problem: how are we going to pass some error code for functions which already return some value. We have several alternatives, each of them with its own set of unwanted consequences:

• as we just saw in the previous example, we can use null to encode an error,
• encode the error codes inside the return value,
• or use an out parameter to store the return value of the method.

Obviously, I wouldn't advise using null, since forgetting a check for null most probably results into a null pointer exception to be raised later in the code. And you are back to square one. Additionally, the exact interpretation of null may sometimes be blurry. Let me give you a concrete example from the Java standard API. Interface Map from java.util has a method get which returns the value mapped to a given key, or null if the key is not mapped to any value. However, and herein lies the ambiguity, if the map permits null values, then null may either represent a mapping or no mapping at all. It is then advised to use method containsKey to distinguish between these cases. What an insecure API! It is safer for the method's user to either disallow null values, or adopt the solutions used by the C# class Dictionary: raise a KeyNotFoundException!
The C# Dictionary interface proposes an alternative way to fetch a value via the TryGetValue method. This method returns a boolean to indicate the presence of a mapping and an additional out parameter to pass the value. This practice has its benefit, which is to avoid the sometimes (but rarely) prohibitive cost of throwing and catching an exception. I will briefly come back to this point later in this post. However, in general, it is not a good practice to modify the state of a parameter of a method which has a return value. Indeed, I believe the separation of methods into either command or query as described in the book Clean Code: A Handbook of Agile Software Craftsmanship to be sane. A quote from this book goes:

Functions should either do something or answer something but not both. Either your function should change the state of an object, or it should return some information about that object.

Sometimes the error code needs to contain more information than just a boolean state. Some coders find it smart to encode the error state in the same type than the one used to store correct values. For instance, if a method normally returns positive integers, then negative integers could be used for error codes. This is a terrible practice which more than often leads the caller to not handling the error code at all.

Contrast these convoluted solutions with the simplicity of wrapping any error information into an exception. Plus you get the stack trace for free!

At last, error codes must necessarily be dealt with by the caller. Sometimes the caller does not have anything reasonable to do. And so she must bear the burden of propagating errors upwards. Not only that, but also, she must be careful not to forget handling any error code. Instead exceptions automatically climb the call stack until they are caught. In the worst case, all exceptions may end in a catch all at the top level of the application. Also, in Java, exceptions which do not inherit RuntimeException must be declared in the method signature. No more silly mistakes!

There may be only one reason to prefer error codes over exceptions. And that is performances issues. Even though using exceptions does not change the complexity of your algorithm, they are said to be very costly both in C# or Java. Well, let us put hard numbers on this belief.
I wrote two versions of a program which tries to retrieve several values from and empty dictionary. The first version throws exceptions:

Dictionary<int, int> dictionary = new Dictionary<int, int>();
int count = 0;
for (int i = 0; i < N; i++)
{
    int result;
    try
    {
        result = dictionary[i];
    }
    catch (KeyNotFoundException)
    {
        result = -1;
    }
    count += result;
}

Whereas the second program returns error codes:

Dictionary<int, int> dictionary = new Dictionary<int, int>();
int count = 0;
for (int i = 0; i < N; i++)
{
    int result;
    if (!dictionary.TryGetValue(i, out result))
    {
        result = -1;
    }
    count += result;
}

I ran both programs with the loop upper bound N successively equal to 1, 2, 4 and 8 million! Here are the durations of each run in milliseconds:

N (in million)	1	2	4	8
Error codes	46	78	202	343
Exceptions	111774	221208	442057	882788

Without any doubt, error codes win the speed test, being roughly 2500 times faster than throwing and catching exceptions. However, keep in mind we are experimenting with exceptions in the million. A quarter of an hour to process 8 million exceptions does not seem outrageous. So unless you are in a case where exceptions are both frequent and on a critical path of your program you shouldn't bother. As always, premature optimization is the root of all evil.

To conclude, I hope I convinced you that using exceptions lets you avoid some code duplication, enables separation of concerns, improves methods API and decreases the risk of forgetting error cases.

To sum up, here are the take-away lessons from this article:

• always prefer exceptions to return codes,
• in Java, try to avoid RuntimeExceptions,
• optimize exceptions out only if really necessary.

Dali, The Temptation of St. Anthony

or exceptions propagating up the program?