A case for code reviews

THIS is a fabulous example why your organization/team should be performing code reviews.

We're not talking about poring over the entire code base once a month (that's just cruel); no, you check the code your teammate or colleague wrote or modified before it even goes into the main branch of the source control repository, just like how open-source projects do it.

Use Review Board: it's free and works with the modern source control systems. If you have one of the source control systems that isn't supported, have one of your teammates come to your desk to give your changes a once-over until you figure out a software solution that allows you to send them the changes so they can review it on their own time while you start working on something else.

Seriously. Stop the bad software madness in its tracks TODAY.

Arrange, Act, Assert

There's a pattern in unit test writing that I noticed a few years back, but it wasn't until recently that I discovered this pattern actually has a name: Arrange, Act, Assert. These represent the three terrifying waves distinct phases of a good unit test: the first part prepares the necessary conditions that simulate a scenario or use-case (arrange), the second invokes the functionality being tested (act) and the third checks that some post-conditions hold (assert).

Here's some Java code I was writing at the time I first noticed the pattern (November 2007):

/**
 * Tests the <i>shuffle</i> method against the Collections.shuffle(List<?>,
 * Random) implementation from which it was derived.
 */
@Test
public void shuffle_AgainstReference ( )
{
    // { initialization
    Random randomSource;
    int length = 20;
    double[] sourceArray = new double[length];
    List<Double> expectedList = new ArrayList<Double> ( length );
    for ( int i = 0; i < length; i++ )
    {
        sourceArray[i] = i;
        expectedList.add ( (double) i );
    }
    // }

    // { double-check
    for ( int i = 0; i < length; i++ )
    {
        String message = "Source array is different at index [" + i + "]";
        assertEquals ( message, expectedList.get ( i ), sourceArray[i] );
    }
    // }

    randomSource = new Random ( 42 );
    ArrayUtil.shuffle ( sourceArray, randomSource );

    randomSource = new Random ( 42 );
    Collections.shuffle ( expectedList, randomSource );

    // { validation
    for ( int i = 0; i < length; i++ )
    {
        String message = "Shuffled array is different at index [" + i + "]";
        assertEquals ( message, expectedList.get ( i ), sourceArray[i] );
    }
    // }
}

That code tests that my implementation of ArrayUtil.shuffle() on an array of doubles works just like the implementation of Collections.shuffle(). One will notice that I called the first block or phase "initialization" and the last one "validation" (which, come to think of it, should have been called "verification" -- more on this at Wikipedia). The block labeled "double-check" should probably have been taken out into its own test.

I recently noticed the more formalized use in the NamedStringFormatSolution.zip project (more about this project at Phil Haack's Named Formats Redux blog post), where the 3 phases of unit testing were explicitly called out by comments in the code:

[Fact]
public void Eval_WithNamedExpressionAndFormat_EvalsPropertyOfExpression()
{
    //arrange
    var expr = new FormatExpression("{foo:#.##}");

    //act
    string result = expr.Eval(new { foo = 1.23456 });

    //assert
    Assert.Equal("1.23", result);
}

The arrange phase is sometimes so trivial that its contents is folded into the act phase, but unless a value is repeated in several tests that it becomes cleaner or less error-prone to extract it out in a constant, it should remain in the test for maximum clarity.

So there you have it: the next time you write a test, design it to execute in three distinct phases of arrangement, acting and assertion. Your test will be better designed, easier to read and other maintainers will thank you for it.