less code, more software

Suppose I'm doing the print-diamond kata in cyber-dojo in Java. I start with a test

 @Test
 public void diamond_A() {
 String[] expected = {
 "A"
 };
 String[] actual = new Diamond('A').toLines();
 assertArrayEquals(expected, actual);
 }

I slime a solution as follows

public class Diamond {
 private char widest;
 public Diamond(char widest) {
 this.widest = widest;
 }
 public String[] toLines() {
 return new String[]{ "A" };
 }
}

now I add a second test

 @Test
 public void diamond_B() {
 String[] expected = {
 " A ",
 "B B",
 " A ",
 };
 String[] actual = new Diamond('B').toLines();
 assertArrayEquals(expected, actual);
 }

and I slime again as follows

public class Diamond {
 private char widest;
 public Diamond(char widest) {
 this.widest = widest;
 }
 public String[] toLines() {
 if (widest == 'A')
 return new String[] { 
 "A" 
 };
 else
 return new String[] {
 " A ",
 "B B",
 " A ",
 };
 }
}

Like all techniques this approach has a certain style. In this case there is a small but definite asymmetry between the specificness of the tests (one for 'A' and another one for 'B') and the slightly less specificness of the code (an explicit if for 'A' but a default everything-else for 'B'). This is a style that is relaxed about the asymmetry, a style that emphasises this step as merely one temporary step on the path of many steps leading towards something more permanent. A style that recognises that code, by it's nature, is always going to be more general than tests.

But suppose you get hit by a bus tomorrow. How easily could your colleagues tell that this code was work in progress? Code that was not finished?

As an experiment I thought I would try an alternative style. One that is not so relaxed about the asymmetry. One that tries a bit harder to be more explicit about distinguishing code that's a temporary step still on the path from code that has reached its destination.

First I wrote a test that expresses the fact that nothing is implemented.

 @Test(expected = ScaffoldingException.class)
 public void scaffolding() {
 new Diamond('Z').toLines();
 }

I make this pass as follows

public class Diamond {
 private char widest;
 public Diamond(char widest) {
 this.widest = widest;
 }
 public String[] toLines() {
 throw new ScaffoldingException("not done");
 }
}

The scaffolding, as its name suggests, will have to be taken down once the code is done. While it remains, it indicates that the code is not done. Now I start. I add the diamond_A test (as before) and make it pass

public class Diamond {
 ...
 public String[] toLines() {
 if (widest == 'A') {
 return new String[]{ "A" };
 }
 throw new ScaffoldingException("not done");
 }
}

I add a second diamond_B test (as before) and make it pass

public class Diamond {
 ...
 public String[] toLines() {
 if (widest == 'A') 
 return new String[] { 
 "A" 
 };
 if (widest == 'B') 
 return new String[] {
 " A ",
 "B B",
 " A ",
 };
 throw new ScaffoldingException("not done");
 }
}

The scaffolding is still there. We haven't finished yet. Now suppose I refactor the slime (by deliberately duplicating) and end up with this

public class Diamond {
 ...
 public String[] toLines() {
 if (widest == 'A') {
 String[] inner = innerDiamond();
 String[] result = new String[inner.length-1];
 int mid = inner.length / 2;
 for (int dst=0,src=0; src != inner.length; src++)
 if (src != mid)
 result[dst++] = inner[src];
 return result;
 } 
 if (widest == 'B') {
 String[] inner = innerDiamond();
 String[] result = new String[inner.length-1];
 int mid = inner.length / 2;
 for (int dst=0,src=0; src != inner.length; src++)
 if (src != mid)
 result[dst++] = inner[src];
 return result;
 }
 throw new ScaffoldingException("not done");
 }
}

The code inside the two if statements is (deliberately) identical so I refactor to this

public class Diamond {
 ...
 public String[] toLines() {
 if (widest == 'A' || widest == 'B') {
 String[] inner = innerDiamond();
 String[] result = new String[inner.length-1];
 int mid = inner.length / 2;
 for (int dst=0,src=0; src != inner.length; src++)
 if (src != mid)
 result[dst++] = inner[src];
 return result;
 } 
 throw new ScaffoldingException("not done");
 }
}

Now I add a new test for 'C'

 @Test
 public void diamond_C() {
 String[] expected = {
 " A ",
 " B B ",
 "C C",
 " B B ",
 " A ",
 };
 String[] actual = new Diamond('C').toLines();
 assertArrayEquals(expected, actual);
 }

This fails. I make it pass by changing the line

 if (widest == 'A' || widest == 'B')

to

 if (widest == 'A' || widest == 'B' || widest == 'C')

Now I remove the if completely

public class Diamond {
 ...
 public String[] toLines() {
 String[] inner = innerDiamond();
 String[] result = new String[inner.length-1];
 int mid = inner.length / 2;
 for (int dst=0,src=0; src != inner.length; src++)
 if (src != mid)
 result[dst++] = inner[src];
 return result;
 throw new ScaffoldingException("not done");
 }
}

And it no longer compiles.
I have unreachable scaffolding.
Time for the scaffolding to come down.
I delete the throw statement.

public class Diamond {
 ...
 public String[] toLines() {
 String[] inner = innerDiamond();
 String[] result = new String[inner.length-1];
 int mid = inner.length / 2;
 for (int dst=0,src=0; src != inner.length; src++)
 if (src != mid)
 result[dst++] = inner[src];
 return result;
 }
}

Now the scaffolding() test fails. I delete that too.
We're green.
The scaffolding is gone.
We're done.

Suppose I've written a test and got it passing...

@Test
public void yahtzee_full_house_scores_25() {
 assertEquals(25, 
 new YahtzeeScorer(4,4,3,4,3).fullHouse());
}

After refactoring I write my next test...

@Test
public void yahtzee_not_full_house_scores_0() {
 assertEquals(0, 
 new YahtzeeScorer(4,4,4,4,4).fullHouse());
}

And this passes first time. That is, it passes without failing first. One of the reasons for failing first is to be sure the test is actually running. For example, suppose I'd forgetten to write @Test and didn't notice that the JUnit output wasn't showing one more test passing. Ooops. A question I've been asked on several occasions is whether, in this situation, you should change the code or the tests to force an initial red. For example, my first version of the yahtzee_not_full_house_scores_0 could have been this, (where I've deliberately used 42 instead of 0 simply because 42 is a good example of a number that is not 0):

@Test
public void yahtzee_not_full_house_scores_0() {
 assertEquals(42, 
 new YahtzeeScorer(4,4,4,4,4).fullHouse());
}

I see it fail, and then change the 42 to 0 and see it pass. This works, and I have done this. Perhaps you have too. If so, do you agree that it doesn't feel right? I've learned that when something doesn't feel right my subconscious is trying to tell me something. I've learned that when I've got two choices it's often a good idea to look for a third. And there is a third way. I could instead start with this:

@Test
public void yahtzee_not_full_house_scores_0() {
 fail("RED-FIRST");
}

And when I've seen it fail, I delete the fail() and write the actual code I want to write:

@Test
public void yahtzee_not_full_house_scores_0() {
 assertEquals(0, 
 new YahtzeeScorer(4,4,4,4,4).fullHouse());
}

Or, as an alternative, I could start with this:

@Test
public void yahtzee_not_full_house_scores_0() {
 fail("RED-FIRST");
 assertEquals(0, 
 new YahtzeeScorer(4,4,4,4,4).fullHouse());
}

and when I've seen it fail I simply delete the fail() line.

Either way I get the mechanics of seeing the fail out of the way and then I write the code as a separate thing. By un-asking the question I avoid having to decide what to temporarily fiddle with - the code or the tests. I get to write the code I actually want to write. All the time.

Here's a small trick which might not be out of place in Michael Feather's excellent book. I'll use Singleton as a example. Suppose you have a class like this (Java):

public class Legacy
{
 public void eg1()
 {
 stuff(Singleton.getInstance().call());
 }
 public void eg2()
 {
 more_stuff(Singleton.getInstance().call());
 }
}

And you want to create a seam for the singleton access. The first step is to introduce two public fields:

public class Legacy
{
 public int Singleton;
 public Singleton instance;
 public void eg1()
 {
 stuff(Singleton.getInstance().call());
 }
 public void eg2()
 {
 more_stuff(Singleton.getInstance().call());
 }
}

Now Lean on the Compiler. All the calls to Singleton.getInstance() no longer compile because the Singleton class is now hidden by the int Singleton just introduced. Next refactor all occurences of Singleton.getInstance() to instance (the other identifier just introduced, you can pick any name for this one but it's type must be Singleton).

public class Legacy
{
 public int Singleton;
 public Singleton instance;
 public void eg1()
 {
 stuff(instance.call());
 }
 public void eg2()
 {
 more_stuff(instance.call());
 }
}

Then Lean on the Compiler again to make sure there's no typos. Finally refactor to this:

public class Legacy
{
 public Singleton instance = Singleton.getInstance();
 public void eg1()
 {
 stuff(instance.call());
 }
 public void eg2()
 {
 more_stuff(instance.call());
 }
}

The name lookup rules for Java allow this to work but I don't think this idea will work in C++ (for example). I haven't got time to try it now as I'm heading off to the NDC conference in Oslo. I only thought of it yesterday. I haven't tried this on real legacy code. Caveat emptor!

Update. I have a C++ version aswell.

Working Effectively With Legacy Code by Michael Feathers (isbn 0-13-117705-2) is a great book. The very first dependency breaking technique (p326) is called Adapt Parameter. It uses this Java example:

public class ARMDispatcher
{
 public void populate(ParameterSource request) {
 String[] values
 = request.getParameters(pageStateName);
 if (values != null && values.length> 0)
 {
 marketBindings.put( 
 pageStateName + getDateStamp(),
 values[0]);
 }
 ... 
 }
 ...
}

Michael writes:

In this class, the populate method accepts an HttpServletRequest as a parameter. ... It would be great to use Extract Interface (362) to make a narrower interface that supplies only the methods we need, but we can't extract an interface from another interface. ...

and a bit later...

Adapt Parameter is one case in which we don't Preserve Signatures (312). Use extra care.

Well, here's a variation on Adapt Parameter where we do Preserve Signatures. I'll stick with the Java example, but the idea is broadly applicable...

First we create our ParameterSource interface and fill it with the signatures of the methods in HttpServletRequest that our populate method calls:

public interface ParameterSource
{
 String[] getParameters(String name);
}

then we implement the adapter for HttpServletRequest:

public class HttpServletRequestParameterSource 
 implements ParameterSource
{
 public HttpServletRequestParameterSource(HttpServletRequest request) {
 this.request = request;
 }
 public String[] getParameters(String name) {
 return request.getParameters(name);
 }
 private HttpServletRequest request;
}

Next we add an overload of populate taking a ParameterSource and implement it with an exact copy-paste:

public class ARMDispatcher
{
 public void populate(ParameterSource request) {
 String[] values
 = request.getParameters(pageStateName);
 if (values != null && values.length> 0)
 {
 marketBindings.put(
 pageStateName + getDateStamp(),
 values[0]);
 }
 ... 
 }
 public void populate(HttpServletRequest request) {
 String[] values
 = request.getParameters(pageStateName);
 if (values != null && values.length> 0)
 {
 marketBindings.put(
 pageStateName + getDateStamp(),
 values[0]);
 }
 ... 
 }
 ...
}

Now we Lean on the Compiler (315) to make sure we haven't missed any methods in HttpServletRequest that our populate method calls. Add any we missed to the ParameterSource interface. Implement them in HttpServletRequestParameterSource as one liners. When it compiles we can refactor to this:

public class ARMDispatcher
{
 public void populate(ParameterSource request) {
 String[] values
 = request.getParameters(pageStateName);
 if (values != null && values.length> 0)
 {
 marketBindings.put(
 pageStateName + getDateStamp(),
 values[0]);
 }
 ... 
 }
 public void populate(HttpServletRequest request) {
 populate(new HttpServletRequestParameterSource(request));
 }
 ...
}

And now we have a seam we can pick it at...

public class FakeParameterSource 
 implements ParameterSource
{
 public String[] values;
 public String[] getParameters(String name) {
 return values;
 }
}

Here's a neat trick Kevlin Henney found in some code Michael Feathers posted at http://pastie.org/534364 Instead of doing this:

ArrayList<Integer> values = new ArrayList<Integer>();
values.add(1);
values.add(2);
values.add(3);

You can do this:

ArrayList<Integer> values = new ArrayList<Integer>() {{
 add(1);
 add(2);
 add(3);
}};

I've been playing with generics in the latest JDK (among other features) to see how many of the features are useful in my eternal JavaSauce project. Quite a few as it happens. Annotations, varargs, new for loop iteration, static imports. And generics. In particular generics for the Visitor Pattern. Here's what I had before Tiger (some class names changed to save typing):

public interface Visitable
{
 void accept(Visitor visitor);
}
public interface Visitor
{
 void visit(A visited);
 void visit(B visited);
 void visit(C visited);
}

These interfaces allow me to visit the A, B, and C classes which all live in the same package and particpate in a Composite relationship (as is often the case). Given the recursive whole-part relationships there's a strong temptation to make the return type boolean instead of void. However, some concrete Visitors don't need or use a return type so for these classes the void return type is preferable. There's a definite tension. Generics can help. Here's what I've now got:

public interface Visitable
{
 <R> R accept(Visitor<R> visitor);
}
public interface Visitor<R>
{
 R visit(A a);
 R visit(B b);
 R visit(C c);
}

One slight problem with this approach is that R can only be a reference type:

public final class SomeVisitor
 implements
 Visitor<boolean> // compile time error
{ ...
}

A small Adapter can help to solve this:

public final class BooleanVisitorAdapter
 implements
 Visitor<Boolean>
{
 public BooleanVisitorAdapter(final BooleanVisitor adaptee)
 {
 this.adaptee = adaptee;
 }
 public Boolean visit(final A visited)
 {
 return Boolean.valueOf(adaptee.visit(visited));
 }
 public Boolean visit(final B visited)
 {
 return Boolean.valueOf(adaptee.visit(visited));
 }
 public Boolean visit(final C visited)
 {
 return Boolean.valueOf(adaptee.visit(visited));
 }
 private final BooleanVisitor adaptee;
}
public interface BooleanVisitor
{
 boolean visit(A visited);
 boolean visit(B visited);
 boolean visit(C visited);
}

Allowing:

someObject.accept(new BooleanVisitorAdapter(
 new BooleanVisitor()
 {
 public boolean visit(final A visited) { ... }
 public boolean visit(final B visited) { ... }
 public boolean visit(final C visited) { ... }
 }));

Note how the adapter creates a Boolean from a boolean. That could be handy; it could avoid the client writing

new Boolean(true)

which would cause needless pressure on the garbage collector (remember this is in a recursive context).

Here's a small fragment I've just spotted in The Elements of Java Style.

if (this.invoiceTotal > 1000.0) {
 this.invoiceTotal = this.invoiceTotal * 0.95;
}

The authors say that the program appears to give a 5 percent discount. And again in the comment (the tip is on writing comments) they point out that:

// Apply a 5% discount to all invoices 
// over one thousand dollars

is a valueless comment because it provides little additional information. Maybe. But it does provide some additional information. I count three things the comment says that the code doesn't. One is the % symbol, two is the word discount, and three is the word dollars. The point is you can say these three things in the software itself. For example:

if (this.invoiceTotal> 1000.0) {
 final double discount = this.invoiceTotal * 0.05;
 this.invoiceTotal = this.invoiceTotal - discount;
}

To my eyes the repetitive this. is just noise, so I would simplify to:

if (invoiceTotal> 1000.0) {
 final double discount = invoiceTotal * 0.05;
 invoiceTotal = invoiceTotal - discount;
}

and then to:

if (invoiceTotal> 1000.0) {
 final double discount = invoiceTotal * 0.05;
 invoiceTotal -= discount;
}

Say it in software. Less code, more software.