TDD is a Design Activity

This has been written and talked about before, and if you've read Kent Beck's TDD book, you'll understand how much it's emphasized, but I still see a lot of test code that is about validation and very little about design. And it doesn't need to be that way. Simply follow the three rules of TDD:

1. Only write production code that makes a failing unit test pass.


2. Write the minimum amount of test code to get production code to fail. Not compiling counts.


3. Write only enough production code to get a test to pass.

What I see the most from the test code I read is violations of rule two. Which is very understandable. Why would you not want to write more test code? Because tests, just like every other piece of the system, have three functions:

1. implement the desired feature


2. afford change


3. reveal intent

Writing line after line of test code to cover every possible corner condition certainly meets the first, but violates the second and third. I've seen test code that looks something like:

public void testServicesStopped() {
  assertServicesStoppedIfEnabled(false, false, false, false, false, false);
  assertServicesStoppedIfEnabled(true, false, false, false, false, false);
  assertServicesStoppedIfEnabled(false, true, false, false, false, false);
  assertServicesStoppedIfEnabled(false, false, true, false, false, false);
  assertServicesStoppedIfEnabled(false, false, false, true, false, false);
  assertServicesStoppedIfEnabled(true, true, false, false, false, false);
  assertServicesStoppedIfEnabled(false, true, true, false, false, false);
  assertServicesStoppedIfEnabled(false, false, true, true, false, false);
  assertServicesStoppedIfEnabled(true, true, true, false, false, false);
  assertServicesStoppedIfEnabled(false, true, true, true, false, false);
  assertServicesStoppedIfEnabled(true, true, true, true, false, false);
  // every possible combination was covered
}

Certainly a very thorough test, but also very very intention hiding. Unit tests should function as documentation for the system. You should be able to read the test and determine what a module does and how it does it. To make matters worse, testServicesStopped() was the first method in the test and assertServicesStoppedIfenabled() was near the bottom with many other tests and helper methods in between. To try and figure out what each boolean meant, you had to scroll up and down the file and try to remember 6 parameters. I would also bet a considerable sum that the bulk of this code was written after the production code was completely finished. Not only does this test code not reveal intent, but it's very rigid. It's hard to change because it's hard to understand. When the customer wants to change how this feature works, and the developer opens this test to write the failing test so he can write the feature changes, he'll have a very difficult time figuring out what to change.

Having a suite of unit tests you can run to verify your system still functions is very important, but just as important is having those unit tests clearly document the system. The easiest way to achieve this is to use TDD the way it was designed to be used by following the Three Laws of TDD.

Lazy Inheritance Bugs

I ran across another type of bug yesterday. I'm calling it the Lazy Inheritance Bug. I'm not sure where I first heard the term lazy inheritance, but I've always thought it was a good way to explain the power of delegation, and until yesterday didn't even realize that it could encompass a category of bugs.

Lazy Inheritance

If you've done any object oriented development, you've probably heard the terms is-a and has-a. Is-a is a way to determine if one object should inherit from another. It's not the best criteria to use, but it is a pretty good rule of thumb. Unfortunately, some developers get into the habit of inheriting from another class just to get some reuse. Some method in a class does some work that they need, so they extend that class to get that work for free. That's lazy inheritance.

Lazy Inheritance Bugs

When you use lazy inheritance, you begin a cascading hierarchy that can result in bugs. You extend some class to get some free work. Someone else extends your class to get some of your free work. Someone extends that class to get more free work until you have some methods very far down the hierarchy that depend on methods way up the hierarchy. Now, the author of the original class changes a method and something completely different breaks. That's the Lazy Inheritance Bug.

And that's why terms like is-a exist. To give you some criteria from which to decide if you should inherit from another class. Barbara Liskov provided a better criteria called the Liskov Substitution Principle (LSP). Basically, given a class A that inherits from class B then any class C that has a reference to class A should also work when given a reference to class B without having to know it has a B. In other words, class B should be substitutable for class A. When you use lazy inheritance, you violate the LSP by definition.

Avoiding lazy inheritance bugs is easy: don't use inheritance unless you can conform to the LSP.

Delegation

Before you use inheritance to get reuse or avoid duplication, take some time to consider delegation instead. Here's an example:

Inheritance:

public class Car {
  public void go() {
    // goes pretty fast
  }
}

public class SportsCar extends Car {
  public void go() {
    // goes really fast
  }
}

Notice the only difference between a Car and a SportsCar is the engine. We could use delegation and only have one type of car.

public interface Engine {
  void go();
}

public class EconomyEngine implements Engine {
  public void go() {
    // goes pretty fast
  }
}

public class SportsEngine implements Engine {
  public void go() {
    // goes really fast
  }
}

public class Car {
  private Engine engine;
  
  public Car(Engine engine) {
    this.engine = engine;
  }

  public void go() {
    engine.go();
  }
}

So now you can configure your car with whatever engine you want. This example doesn't seem to hold its weight. There's still inheritance going on, but now it's with Engine and not Car. What gives? Well, just add one more option to see how delegation almost always wins:

public class FamilyCar {
 public void go() {
   // goes pretty fast
 }

 public void turn() {
   // turns OK
 }
}

public class SportsCar {
 public void go() {
   // goes really fast
 }

 public void turn() {
   // turns really well
 }
}

public class SportSuv {
 public void go() {
   // goes really fast
 }

 public void turn() {
   // turns OK
 }
}

public class EconomyCar {
 public void go() {
   // goes pretty fast
 }

 public void turn() {
   // turns really well
 }
}

How can we use inheritance to combine just these two features? With delegation it's easy:

public interface Engine {
 void go();
}

public class SportsEngine implements Engine {
 public void go() {
   // goes really fast
 }
}

public class EconomyEngine implements Engine {
 public void go() {
   // goes pretty fast
 }
}

public interface Suspension {
  void turn();
}

public class SportsSuspension implements Suspension {
  public void turn() {
    // turns really well
  }
}

public class EconomySuspension implements Suspension {
  public void turn() {
    // turns OK
  }
}

public class Car {
  private Engine engine;
  private Suspension suspension;
  public Car(Engine engine, Suspension suspension) {
    this.engine = engine;
    this.suspension = suspension;
  }

  pubic void go() {
    engine.go();
  }

  public void turn() {
    suspension.turn();
  }
}

Now we can configure any Car with any type of Engine and Suspension. When you start thinking about the braking system and traction control and air bags and body style and on and on, you can see that trying to use inheritance for each possible car configuration would get out of hand really fast. Delegation allows you to configure your car with each component and have the reuse in the components and not in the hierarchy. And that's how to squash lazy inheritance bugs before they hatch.

Naming the Flag Object

So, you've decided to avoid the If Bug by using the Flag Object, but you can't seem to come up with a good name. Names should come from natural mappings not artificial ones. By that I mean the names of your objects should come from the customer's not the developers' vocabulary. Listen closely to what the customer says when she describes the story she wants implemented. When I asked my fictional customer,

"How are managers' pay calculated differently from regular employees' pay?"

she said,

"Managers and employees are paid based on different pay scales."

Now, I could have just listened to what she meant and gone about implementing a PayCalculator, but I listened closely to what she said and the words pay scale jumped out at me. That's how PayScale was born. Notice I chose a noun.

Avoid words which do not come from either the customer or the developer domains. "ER" words are the worst: Manager, Calculator, Helper etc.. These words hide the intent of your object, make it hard for other developers to trace its responsibility back to a user story, and lay the groundwork for the object to become large. How many small, cohesive, single responsibility FooManager classes have you ever seen?

When the customer comes to me later and says,

"We've changed the manger's pay scale to take into account how many employees each manager manages,"

I can go straight to the ManagerPayScale object and implement the changes instead of trying to figure out if the change is in the PayManager or EmployeePayCheckUtility or SuperPayRateHelper. The words manager and pay scale were written right in the feature request, so when I look in the paycheck package and see an object called ManagerPayScale chances are I won't have to look much further.

Names are very very important in development. Object names, variable names, and method names can make your code easy to read, intent revealing, and pleasant to work with. On the other hand, poor names hide intent, make code difficult to understand, and painful to maintain. Put effort into choosing your names. And when you get to the refactoring stage, reevaluate your names. Sometimes they make perfect sense when you first came up with them, but later, in the full context of the code, they don't. So change them. If you are not in a situation in which you get to pair program, show your names to other developers. The names might have made perfect sense to you, but others are going to have to maintain your code, so let them have some input.

Name your objects well and not only will other's enjoy maintaining your code, you'll have fewer bugs.

Flag Object

I've worked with a lot of programmers that write if bugs, so I've come up with small simple solutions. The simplest solution I've come up with so far for "if bugs" is what I call the Flag Object.

The Algorithm

1. Instead of making a boolean, make an object.


2. Take the if clause from the client code and make it a method on the object.


3. In the client code call the method on the object.

I said it was simple.

An Example

The Old Code

public class Employee {
private boolean manager;
public boolean isManager() {
  return manager;
}
...
}

...
Employee employee = new Employee();
if (employee.isManager()) {
// calculate manager pay
} else {
// calculate employee pay
}
...

This is a potential "if bug". The following is better and only slightly more 'work':

public class Employee {
 private PayScale payscale;
 public double calculatePay() {
   payscale.calculatePay();
 }
...
}

public interface PayScale {
 double calculatePay();
}

public EmployeePayScale implements PayScale {
 public double calculatePay() {
   // calculate employee pay
 }
}

public ManagerPayScale implements PayScale {
 public double calculatePay() {
   // calculate manager pay
 }
}

...
Employee employee = new Employee()
double pay = employee.calculatePay()
..

If you were doing the old boolean flag trick, you'd have places in the code to set the boolean true or false.

employee.setManager(true);

Now you call something like:

employee.setPayScale(new ManagerPayScale());

Now the details of how a manager's and employee's pay is calculated is encapsulated in the PayScale objects. How pay is calculated can change and not affect anyone who actually uses the code to calculate pay. You can also add new employee types and not have to change existing code. That means the code satisfies the Open-Closed Principle.

There a few things I don't like. One, the object that calls

employee.setPayScale()

depends on the low level objects in the PayScale hierarchy. This breaks the Dependency-Inversion Principle that says concrete objects should depend on abstractions. Preferably the employee object is obtained from some kind of factory that knows how to make Employee objects already configured with the appropriate PayScale object. However, it's better than the "if bug" problem so I can live with a little dependency issue. At least the dependency issue is localized and easily solved. If Bugs can be spread throughout the code and finding all of them very very difficult. I also prefer Flag Objects because they conform to Tell Don't Ask.

If Bugs

I've run across a particularly nasty type of bug, and I really haven't seen anyone else talk about it, so I will. I call them "if bugs" and they occur when encapsulation is broken. They usually take the form of:

public class Employee {
 private boolean manager;

 public boolean isManager() {
  return manager;
 }
...
}

...
if (employee.isManager()) {
 calculateManagerPay();
else {
 calculatePay();
}
...

The problem is that now there are potentially many other parts of the system that care about whether an employee is a manager or not. Some of those other parts will not be changed, and defects will be reported against the system when those parts of the application are used. That's the "if bug".

Encapsulation was broken when manager was exposed to the rest of the system. Object Oriented Design tells us to not expose the implementation details of a class to the rest of the system.

There are many better ways to implement this, and I'll save what is perhaps the simplest way, Object Flag, for next time.