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?

Jenkins: trend is destiny

Introduction

Every project is driven by the strong demand for more features. Be it the evil manager (who juggles with three mobile phones to extend unattainable deadlines), the evil boss (who signs your pay-check), or the evil customer (who ultimately funds the project), they all want more and different. Left to these forces alone, any project rapidly decays in contradictory layers of increasing complexity. Hopefully, as experienced developers we all know better: by constantly capitalising our progress with non-regression tests and fighting entropy through code refactoring.

It is a good thing to know where to go, it is even better to know where we stand. And this post is just about that: how to quickly grasp and continuously monitor the global state of your software project. To do so, I will show you how to configure a job in Jenkins. Jenkins is an open source continuous integration server. In more mundane terms, it allows you to :

• periodically run tasks on the latest version of the code base,
• access the results through a web interface. 

Here are some of the things Jenkins can do for you:

• build the project,
• run all non-regression test suites,
• run static analysis tools,
• present tool results,
• plot metric trends (line count, test count, warning count...),
• package the product.

So at all times you get a synthetic view of your project, its general health and the progress being made. Since Jenkins can even package your software into a product, you can almost ship it any time! As I hope to demonstrate in this tutorial, Jenkins has a low initial investment cost (which can moreover be gradually paid) and then runs all by itself. It is for the most part extremely intuitive.

Tutorial

I will be working with Ubuntu (Oneiric Ocelot). Since Jenkins is a Java application, most of the tutorial should still apply with other platforms. However this post assumes you know how to install software packages for your distribution (with apt-get, aptitude or synaptic for instance). In order to install Jenkins the easiest way is to get it from the Ubuntu Software center. With older versions of Ubuntu, you may need to first add Jenkins repository to your software sources and then install with aptitude:

wget -q -O - http://pkg.jenkins-ci.org/debian/jenkins-ci.org.key | sudo apt-key add -
sudo sh -c 'echo deb http://pkg.jenkins-ci.org/debian binary/ > /etc/apt/sources.list.d/jenkins.list'
sudo aptitude update
sudo aptitude install jenkins

This page explains the installation process in a more detailed way. Now, you should be able to access Jenkins web interface by following this link http://localhost:8080/. If this does not work, then maybe you need to start the service first:

sudo service jenkins start

As a running example, I will be using Hime Parser Generator. As indicated by its name, Hime automatically generates C# parsers from context free grammars (much like yacc). But really, the purpose of Hime is irrelevant to the discussion of setting up a job in Jenkins. Hime is written in C#. This will not cause any problem. In fact, Jenkins is language agnostic, even though many of its plugins are targeted for the Java world.

A first build

Let us create our first job. To do so, simply click on New Job from the Jenkins entrance page. Name the job Hime.Parser. The job data is going to be stored on disk in directory:

/var/lib/jenkins/jobs/Hime.Parser

From experience, it is preferable to avoid spaces in job names. Otherwise, some Jenkins plugins may display awkward behaviours. Then select the 'Build a free-style software project' option. Click OK. You will be brought to the configuration page of the newly created job. Let us ignore this page for the time being and simply, scroll downward to click Save. You are now at the main control page of the job. For now, options, listed on the left-side, are fairly limited, basically we can either Delete Projet or Build Now. Let us launch our first build. The first build, build #1, should soon appear with its status (a blue ball) and build date in the 'Build History' box.

Checking out the code

For the second build we are going to fetch the project source code from its repository. Hime code is hosted on codeplex, and it is tracked with Subversion. (even though I would prefer it to be running on Mercurial). Let us go back to our job's configuration page and choose Subversion in the 'Source Code Management' section. Then, we enter the following as its 'Repository URL':

https://himeparser.svn.codeplex.com/svn/Hime/trunk

Next select 'Emulate clean checkout by first deleting unversioned/ignored files, then 'svn update'' as the 'Check-out Strategy'. This strategy is safer than just use svn update as it will start each build with a clean repository, but largely more efficient than always checking out a full-blown fresh copy. Then save and launch a new build. While it is running, you can click on the build and check the Console Output to follow what is going on. You should see something like:

Started by user anonymous
Checking out a fresh workspace because there's no workspace at /var/lib/jenkins/jobs/Hime.Parser/workspace
Cleaning workspace /var/lib/jenkins/jobs/Hime.Parser/workspace
Checking out https://himeparser.svn.codeplex.com/svn/Hime/trunk
A Hime
A Hime/trunk
A Hime/trunk/Exe.HimeCC
...
A Hime/trunk/Tests/Tests.csproj
A Hime/trunk/TraceAndTestImpact.testsettings
A Hime/trunk/VersionInfo.cs
At revision 11108 Finished:
SUCCESS

From the job main page, you can now browse the project source code by clicking on Workspace.

Tracking line count and installing plugins

Now we are going to use Jenkins in order to track a very simple yet important metric: line count. To do so, we will use the external program sloccount and the corresponding Jenkins plugin. Before going any further, let me first explain the Jenkins execution model. Each job execution is split in two major phase: 'Build' and 'Post-Build'. During the 'Build' phase external tools are called and may produce results as files (mostly .xml but any format is possible). Once the 'Build' phase completes, the 'Post-build Actions' execute. Most of them read some result file produced during the previous phase and convert it in visual elements to display in the web interface. Jenkins has a great variety of plugins for build actions, for instance shell, python, ruby, ant (you name it...) scripts, and for post-build actions.
Plugins are managed in the 'Update Center' which can be accessed from Jenkins entrance page by clicking on Manage Jenkins, then on Manage Plugins and at last on the 'Available' tab. For the time being, select Jenkins SLOCCount Plug-in from the list of all available plugins. (If there seems to be no plugin available, then you may need to check for updates manually. This can be done in the 'Advanced' tab). Install and restart Jenkins.
If you now go to the configuration page of the Hime.Parser, you may notice option 'Publish SLOCCount analysis results' which appeared among the 'Post-build Actions'. Tick it. In addition, we must obviously invoke sloccount. In the 'Build' section, add a build step of type 'Execute shell' with the following command:

sloccount --details --wide --duplicates VersionInfo.cs Lib.CentralDogma Exe.HimeDemo Lib.Redist Exe.HimeCC > sloccount.sc

As you may well know, sloccount counts .xml files too. So it is a good idea to run it as one of the first build step. At this point, the workspace is still clear from any .xml result files that may be produced by other build steps.
Save the new configuration and launch a build. A new link SLOCCount, should have appeared in the left menu. If you click it you will get to a sortable table which displays all file sizes. If you build a second time (and refresh the page) then Jenkins plots the graph of total program size per build in the main page of the job. For now it should evidently show a flat trend.

Compiling and building regularly
Again back to the configuration page, this time to set up a whole project compilation. We add a shell script in the 'Build' section which uses mdtool to compile all projects:

mdtool build -p:Lib.Redist -c:Release

mdtool build -p:Lib.CentralDogma -c:Release

mdtool build -p:Exe.HimeDemo -c:Release

mdtool build -p:Exe.HimeCC -c:Release

mdtool build -p:Tests -c:Release

Until now we launched all our builds manually. It would be more convenient to have Jenkins start a build every time the code changes. So we turn to the 'Build Triggers' section. We select 'Poll SCM' and have Jenkins poll the subversion repository every quarter-hour. Following the cron syntax:

*/15 * * * *

If you want more details about the syntax, just click on the question mark next to the field. Jenkins offers a very comprehensive help. Some people prefer to build at regular intervals of time, such as every night. This is done by opting for 'Build periodically'. Also notice the 'Build after other projects are built' which is useful when your software product is composed of several small projects which depend on each other.
Save and launch a build. If you like, you may check the mdtool compiler progress in the Console Output. This time click on trend next to the 'Build History'. This page displays a 'Timeline' with all builds and plots each build duration. As you can check, the last build lasts much longer than the previous ones.

At this point, Jenkins benefit is still modest. However, it will warn you by a build failure every time a code modification broke the compilation process.

Executing non-regression tests

This is probably the most important part of continuous builds: running tests frequently. Remember how we just compiled all projects in Hime. Among them was project Tests which contains all nunit test suites. We will run them with nunit-console and then let Jenkins display the results. First, let us install the Jenkins NUnit plugin from the Update Center. Then go back to the configuration page of the job. Add a new shell in the 'Build' section with the following content:

nunit-console -noshadow Tests/bin/Release/Tests.dll

And, fill the 'Publish NUnit test result report' in the 'Post-build Actions' with:

TestResult.xml

Now build, twice. If you reload the page after the builds complete, a graph with 'Test Result Trends' should appear. This graph plots the number of tests over time. Successful tests are in blue, ignored in yellow and failed in red. If you click on the graph you will browse down the tests hierarchy and see the duration and status of each test.

Displaying Static analysis reports

Back to the Update Center, let us install plugins to display results of static analysis tools. In order to get the list of compiler warnings, the list of tasks (TODO, FIXME, XXX... comments present in code) and the results of Gendarme, we need to install plugins Warnings Plug-in, Task Scanner Plugin, Jenkins Violations plugin and Static Code Analysis Plug-ins.
Then we set up each build step from, as usual, the configuration page of the Hime.Parser job.
For compiler warnings, select 'Scan for compiler warnings' among the 'Post-build Actions'. There is no ad-hoc parser for mdtool outputs, so we choose 'MSBuild' to 'Scan console log'.
Then, for tasks, select 'Scan workspace for open tasks' and fill 'Files to scan' with a fileset representing all C# source files:

**/*.cs

Also, fill 'Tasks tags' with TODO as a 'Normal priority'.

As for Gendarme, it is first run in a shell script with the following command:

gendarme --config gendarmeConfig.xml --xml gendarmeResults.xml Exe.HimeCC/bin/Release/himecc.exe Exe.HimeDemo/bin/Release/HimeDemo.exe Lib.Redist/bin/Release/Hime.Redist.dll Tests/bin/Release/Tests.dll Lib.CentralDogma/bin/Release/Hime.CentralDogma.dll || exit 0

Note that we are using rule filters stored in file gendarmeConfig.xml on the Hime repository. Also, the command needs to be combined with an exit 0, in order not to fail the build on the first Gendarme warning. In the 'Post-build Actions' section, tick 'Report Violations' and fill the gendarme 'XML filename patter' with:

gendarmeResults.xml

We are now ready to save, build (twice again to get trends) and check the result. We get three new trend graphs, all click-able. Clicking on 'Open Tasks Trend' brings a list of all open tasks in the project down to the source file. Similarly for 'Compiler Warnings Trend' and 'gendarme'. Note that the MSBuild parser sometimes fails to extract the correct file name or warning description from the compilation outptut. This is because of line breaks that mdtool inserts in order to keep column-width under a certain limit. I would really like to know how to avoid this bug...

Packaging

The last action of this tutorial will be to add a minimal packaging process to our job. It will simply create a zip file with all the content necessary for end-product delivery. Back to adding a new shell script in the configuration page:

mkdir himecc_latest
cp Exe.HimeCC/bin/Release/* himecc_latest
cp Lib.Redist/bin/Release/Hime.Redist.xml himecc_latest
zip -r himecc_latest.zip himecc_latest

Then tell Jenkins it should present the resulting archive on the main page of the job by clicking the post-build action 'Archive the artefacts'. Set himecc_latest.zip as the name of 'Files to archive'. Let us build one last time and check the downloadable artefact.
  

Going further
By now, you must have gotten a fairly good grip on Jenkins. Going further is pretty easy and can be done by gradual improvements. For instance, having test coverage trends is pretty necessary and so are code duplication detection warnings....
Here are some plug-ins which may prove useful:

• Jenkins disk-usage plugin: lets you monitor the disk usage of jobs. In the configuration page, you can tell Jenkins to discard plug-ins when they are too old or too many. 
• Downstream build view: necessary when your whole product in split in several small projects,
• Plot plugin: to display your own metrics (like the number of tasks in your project tracker for the current deadline),
  
• Green Balls: just better than blue,
• Safe Restart Plugin,
• Hudson Status Monitor,
• ChuckNorris Plugin: I will let you figure out this one by yourself.

Conclusion
I have found that the two most important trends to follow in any project are its line and test count. Test count needs to increase while line count should decrease relentlessly. Keeping these two trends steady, is the best recipe to attain (almost) flawless software. Doing so, you may realise how much trend is, in fact, destiny.

Visualising important code metrics in Jenkins, creates a positive feedback loop: the impact of any code modification can be, almost immediately, sensed and the project stays the course.

Here are some more metrics, that can not, to the best of my knowledge, be tracked in Jenkins and that would be great to have:

• More information from the bug-tracker, such as the time taken to complete each issue, the number of implemented and remaining features...
• The time elapsed between the discovery of two consecutive bugs,
• The ability to sort tasks (TODO comments) according to their age (i.e. the number of commits they have been in existence).

Here is my set of questions for you today:

• Do you use a continuous integration server?
• Which one do you prefer?
• What about sonar?
• If you are using Jenkins, which plug-ins do you find helpful?

In a later post, I would like to talk about the following topics:

• How to order Jenkins with a script using its command-line interface,
• How to write your own plug-in for Jenkins, in particular to follow tests coverage statistics as computed by opencover. 

Some metrics for project Gaia. Trend is destiny?

Testing internal classes: quality and minimal interface at the same time

Today, is a brief entry. I just found a very satisfactory answer to one of my everlasting questions about non-regression testing. Namely, as stated in this previous post, how is it possible to test internal classes while keeping all nunit test code in a separate project?

Well, there is an attribute from System.Runtime.CompilerServices, that lets you do just that. Let's say your test project is called TestSuite, then all you need to do is add the following line to AssemblyInfo.cs in the project under test:

[assembly: InternalsVisibleTo("TestSuite")]

Now all internal classes and methods are visible to the test methods in TestSuite. So you can do deep non-regression testing of an assembly and still present a minimal interface to your users. Pretty neat, isn't it?

If I am not wrong, this problem does not arise in Java. In Java, the notion equivalent to assembly-internal is package-internal. So even though junit code is in a distinct project, it suffices to declare it as part of the same package as the project under test in order to gain full access to all package-internal classes and methods.

As often, stackoverflow has it all: this answer originates from there.

René Magritte, Decalcomania