Training big refactorings with code katas

Recently, we had to refactor some methods out of a class into a new class. In the past, I already made some big refactorings saved by unit tests. Retrospectively, I must admit that my unit tests back then weren't unit tests but mainly integration tests: although I stubbed away the serial communication, my tests used always all layers of the program. So, instead of mocking C when testing D-->C (uses), I mocked A in D-->C-->B-->A. That means, if you want to refactor B in this chain to B'-->B'', you have to adjust all tests of C and D respectively. This is of course cumbersome.

Thank god, I almost always mock the next layer now, so that refactoring the tests is manageable. I even succeeded to refactor the tests step by step in a TDD-manner having mostly only 1 red test. To be able to do that, the Bank OCR-kata has helped me a lot. In this kata, you can do two class-refactor-steps in the first user story. I would like to present the second one in respect to clearly refactor the tests also (it helps a lot if you're familiar with this kata, so feel free to dive into it or even do a test-run for your own before reading further on).

Code so far:

public class BankAccountParser
{
  private readonly IBankAccountSplitter _bankAccountSplitter;

  public BankAccountParser(IBankAccountSplitter bankAccountSplitter)
  {
    if (bankAccountSplitter == null)
      throw new ArgumentNullException("bankAccountSplitter");

    _bankAccountSplitter = bankAccountSplitter;
  }

  public string Parse(string bankAccount)
  {
    if (string.IsNullOrEmpty(bankAccount))
      throw new ArgumentNullException("bankAccount");

    var bankAccountNumber = new StringBuilder();

    foreach (var digit in _bankAccountSplitter.Split(bankAccount))
    {
      if (digit == " _ | ||_|")
      {
        bankAccountNumber.Append("0");
      }
      else
      {
        bankAccountNumber.Append("1");
      }
    }

    return bankAccountNumber.ToString();
  }
}

Tests so far:

[TestFixture]
public class BankAccountParserTests
{
  private BankAccountParser _target;
  private IBankAccountSplitter _bankAccountSplitter;

  [SetUp]
  public void Setup()
  {
    _bankAccountSplitter = MockRepository.GenerateStub<IBankAccountSplitter>();
    _target = new BankAccountParser(_bankAccountSplitter);
  }

  [Test]
  public void Ctor_ObjectIsNotNull()
  {
    Assert.IsNotNull(new BankAccountParser(_bankAccountSplitter,
MockRepository.GenerateStub<IDigitParser>()));
  }

  [Test]
  public void Ctor_BankAccountSplitterIsNull_ThrowArgumentNullException()
  {
    Assert.Throws<ArgumentNullException>(() => new BankAccountParser(null));
  }

  [Test]
  public void Parse_Null_ThrowsArgumentNullException()
  {
    Assert.Throws<ArgumentNullException>(() => _target.Parse(null));
  }

  [Test]
  public void Parse_Zeros()
  {
    var zeros = " _  _  _  _  _  _  _  _  _ \n" +
                "| || || || || || || || || |\n" +
                "|_||_||_||_||_||_||_||_||_|\n" +
                "                           \n";

    _bankAccountSplitter.Stub(x => x.Split(zeros)).Return(new List<String>
    {
      " _ | ||_|",
      " _ | ||_|",
      " _ | ||_|",
      " _ | ||_|",
      " _ | ||_|",
      " _ | ||_|",
      " _ | ||_|",
      " _ | ||_|",
      " _ | ||_|"
    });

    var actual = _target.Parse(zeros);

    Assert.That(actual, Is.EqualTo("000000000"));
  }

  [Test]
  public void Parse_AOneAndZeros()
  {
    var oneAndZeros = "    _  _  _  _  _  _  _  _ \n" +
                      "  || || || || || || || || |\n" +
                      "  ||_||_||_||_||_||_||_||_|\n" +
                      "                           \n";

    _bankAccountSplitter.Stub(x => x.Split(oneAndZeros)).Return(new List<String>
    {
      "     |  |",
      " _ | ||_|",
      " _ | ||_|",
      " _ | ||_|",
      " _ | ||_|",
      " _ | ||_|",
      " _ | ||_|",
      " _ | ||_|",
      " _ | ||_|"
    });

    var actual = _target.Parse(oneAndZeros);

    Assert.That(actual, Is.EqualTo("100000000"));
  }
}

Now I would like to introduce a DigitParser-class which should parse one digit and return a 1-string-representation of it. Therefore, I introduce the interface for it:

public interface IDigitParser
{
  string Parse(string bankAccountDigit);
}

Next, I need to inject it into the target. To do that, I take the easiest test and rewrite it using the new interface:

[Test]
public void Ctor_ObjectIsNotNull()
{
  Assert.IsNotNull(new BankAccountParser(_bankAccountSplitter,
                                         MockRepository.GenerateStub<IDigitParser>()));
}

To make it green, just complete the constructor:

public BankAccountParser(IBankAccountSplitter bankAccountSplitter, IDigitParser digitParser)
{
  if (bankAccountSplitter == null)
    throw new ArgumentNullException("bankAccountSplitter");

  _bankAccountSplitter = bankAccountSplitter;
}

This makes all other tests red because they do not compile anymore. So, alter the setup-method

[SetUp]
public void Setup()
{
  _bankAccountSplitter = MockRepository.GenerateStub<IBankAccountSplitter>();
  _digitParser = MockRepository.GenerateStub<IDigitParser>();

  _target = new BankAccountParser(_bankAccountSplitter, _digitParser);
}

and one test:

[Test]
public void Ctor_BankAccountSplitterIsNull_ThrowArgumentNullException()
{
  Assert.Throws<ArgumentNullException>(() =>
    new BankAccountParser(null, MockRepository.GenerateStub<IDigitParser>()));
}

We're green again and ready for a next test: Assert that when passing in a null-DigitParser, an ArgumentNullException will be thrown.

[Test]
public void Ctor_DigitParserIsNull_ThrowArgumentNullException()
{
  Assert.Throws<ArgumentNullException>(() =>
    new BankAccountParser(_bankAccountSplitter, null));
}

This test is red and becomes green with following code:

public BankAccountParser(IBankAccountSplitter bankAccountSplitter, IDigitParser digitParser)
{
  if (bankAccountSplitter == null)
    throw new ArgumentNullException("bankAccountSplitter");

  if (digitParser == null)
    throw new ArgumentNullException("digitParser");

  _bankAccountSplitter = bankAccountSplitter;
  _digitParser = digitParser;
}

Now comes an interesting step, because we insert an hint into one test how to refactor the sut:

[Test]
public void Parse_Zeros()
{
  var zeros = " _  _  _  _  _  _  _  _  _ \n" +
              "| || || || || || || || || |\n" +
              "|_||_||_||_||_||_||_||_||_|\n" +
              "                           \n";

  _bankAccountSplitter.Stub(x => x.Split(zeros)).Return(new List<string>
  {
    " _ | ||_|",
    " _ | ||_|",
    " _ | ||_|",
    " _ | ||_|",
    " _ | ||_|",
    " _ | ||_|",
    " _ | ||_|",
    " _ | ||_|",
    " _ | ||_|",
  });
  _digitParser.Stub(x => x.Parse(" _ | ||_|")).Return("0");

  var actual = _target.Parse(zeros);

  Assert.That(actual, Is.EqualTo("000000000"));
}

All tests are still green, so we're ready to refactor:

public string Parse(string bankAccount)
{
  if (string.IsNullOrEmpty(bankAccount))
    throw new ArgumentNullException("bankAccount");

  var bankAccountNumber = new StringBuilder();

  foreach (var digit in _bankAccountSplitter.Split(bankAccount))
  {
    if (!string.IsNullOrEmpty(_digitParser.Parse(digit)))
    {
      bankAccountNumber.Append(_digitParser.Parse(digit));
    }
    else
    {
      if (digit == " _ | ||_|")
      {
        bankAccountNumber.Append("0");
      }
      else
      {
        bankAccountNumber.Append("1");
      }
    }
  }

  return bankAccountNumber.ToString();
}

Great, but this can be rewritten easier:

public string Parse(string bankAccount)
{
  if (string.IsNullOrEmpty(bankAccount))
    throw new ArgumentNullException("bankAccount");

  var bankAccountNumber = new StringBuilder();

  foreach (var digit in _bankAccountSplitter.Split(bankAccount))
  {
    bankAccountNumber.Append(string.IsNullOrEmpty(_digitParser.Parse(digit)) ?
      "1" :
      _digitParser.Parse(digit));
  }

  return bankAccountNumber.ToString();
}

But now, the last test is red. We haven't stubbed the DigitParser there yet. To do this, I move the stubbing into the setup-method and also add the canned answer needed for the last test:

[SetUp]
public void Setup()
{
  _bankAccountSplitter = MockRepository.GenerateStub<IBankAccountSplitter>();

  _digitParser = MockRepository.GenerateStub<IDigitParser>();
  _digitParser.Stub(x => x.Parse(" _ | ||_|")).Return("0");
  _digitParser.Stub(x => x.Parse("     |  |")).Return("1");

  _target = new BankAccountParser(_bankAccountSplitter, _digitParser);
}

Prepared like this, we tweak the production code a last time ending with

public string Parse(string bankAccount)
{
  if (string.IsNullOrEmpty(bankAccount))
    throw new ArgumentNullException("bankAccount");

  var bankAccountNumber = new StringBuilder();

  foreach (var digit in _bankAccountSplitter.Split(bankAccount))
  {
    bankAccountNumber.Append( _digitParser.Parse(digit));
  }

  return bankAccountNumber.ToString();
}

But this is not the end for the tests. Actually, we're just testing if the BankAccountSplitter and the DigitParser are working together correctly. So, instead of the last two tests, we should write:

[Test]
public void Parse_SplittedInto2Digits_ConcatenateDigits()
{
  _bankAccountSplitter.Stub(x => x.Split(null)).
    IgnoreArguments().
    Return(new List<string>
    {
      "foo",
      "bar"
    });

  _digitParser.Stub(x => x.Parse("foo")).Return("a");
  _digitParser.Stub(x => x.Parse("bar")).Return("b");

  var actual = _target.Parse("nonrelevant");

  Assert.That(actual, Is.EqualTo("ab"));
}

This points us to some more tests, I had forgotten initially like what happens when the DigitParser returns null or throws an error? What if the BankAccountSplitter has this behavior?

This scales pretty well to "real life" code and as said before, the training with this kata has helped me recognize this pattern during my work and the usefulness of it.

Coding dojo helper - Mortal Kombat Edition

A colleague of mine told us that really good developers could do a red-green-refactor-circle in about 90 seconds. This sounds quite fast, but I thought that maybe one day we could achieve that... To help getting a feeling for the time spend doing such a circle, I wrote a tool. This tool should take the time of each developer doing a round and calculate the average time of a complete timebox used for a kata, for instance.

Learned new tools :)

As a kata originally comes from martial arts, the tool got a touch of the famous video game Mortal Kombat ^^ For development, I used Blend, Gimp and Audacity to do the design and learned some new tricks. For example, one can draw an ellipse with Blend and select Object --> Path to convert it into an animation path. This path can be used in a storyboard to make a button go round another control for example:

The second thing I've learned is to capture keystrokes even if the window isn't active. This article of Stephen Toub has shown me how to do that. So, if the user presses the Scroll-Button, the stopwatch starts. If he has finished his work, he presses the Scroll-Button again and the next developer begins to code. At the end of the timebox, one presses the Pause-Button and the screen changes to show the average TDD-time and a chart listing all times:

Neat, he? This is also the first time I used a DCVS.

The first attempt was to use Git, GitHub and TortoiseGIT as mediator. But as I'm developing this on a company-laptop during my ride home, there were some difficulties accessing GitHub and I abandoned this quite fast (I do think there are ways to circumvent this, but I hadn't got the nerves to look for them any further). So I tried Mercurial, Kiln and TortoiseHg and it worked like a charm. If you want, you can fetch the sourcecode at codingdojo.kilnhg.com and have a try for yourself if you can master the red-green-refactor-mantra in under 90 seconds. On our first try, we set the alarm to a moderate 3-min-timeout, but did a 4:30 min in average *ouch*. Next time, we'll use a 4-min-timeout and are keen to beat that :)

Coding dojos

At work, we do weekly coding dojos since 2 1/2 months now - and it's great!

Every thursday at 11am we meet in front of a computer and start a 1h-timeboxed session. This session consists of 3 parts: discuss kata, code, retrospective.

Discuss kata

First, we decide which kata to start with. If we haven't finished the kata from the week before, we throw away the results and start again from scratch including finding a common interpretation of the requirements (e.g. does an account number consist of 3 or 4 lines). Throwing away the work you have already done is not painful at all. Actually, it is necessary to improve yourself. Seeing direct improvements (e.g. completing a part of the kata which was never touched the week before) is priceless!

This part should be as long as necessary but as short as possible. At the moment, we need more than 10 minutes before writing our first test. I think we can cut that into halves in the coming weeks.

Code

At the moment, we work on the Bank OCR-kata and haven't finished the first user story yet. Before this kata, we successfully finished the katas Prime Factors and Roman Numerals *proud*. We do the katas (as well as our actual real-code-pair-programming) in a flow-Randori system:
The first person to be on the keyboard creates the test-project and writes the first test. After that, we play a round of musical chairs and the next developer solves the test. When it's green, he has the chance to refactor the code using baby-steps. Having finished the refactoring, all he has to do now is to write the next test and initiate the next round of musical chairs (which doesn't necessarily needs music to play in the background).
I think doing this kind of system is perfect when you're 4-6 people. Are there more than that, you would probably more likely do the prepari system. But being 10+ developers, I would rather split the group into two halves than having one big group where not everyone can participate.

Retrospective
About 5 minutes before the deadline, we begin the retrospective and try to find out what was good or what can be better next time. Right now, we're far away from a fast TDD-circle: approximately 5 minutes lasts a red-green-refactor-mantra, which surely can be lowered to 2 minutes.

In my opinion, every company writing software should have a regular coding dojo in the calendar. It helps enormously to spread a common coding style. Everyone learns from everyone, be it new keyboard-shortcuts (- is my newest) or amazing tools like R# or Visual Studio Power Tools.

With coding dojos, it is easy to integrate new team members and guide them in learning TDD. All in all, I highly recommend that!

Code katas

I just finished my first code kata :) I did FizzBuzz, a very easy one just to get started. I wrote the last code-bit for the last test to past in a quarter of an hour. Therefore, I continued with "stage 2" of the kata.

I wanted to make it easy to extend and got lost half the way :( There was a point where I refactored too much without unit testing. This was bad because the bigger the steps the more likely it is you have to go a step backwards.

Luckily, I got my act together and accomplished my plan. The rest was fine and all tests passed.

On a second try, my goal is to don't use the debugger anymore (I needed it twice) and take baby steps.

Pair Programming

Puh, just finished a pair programming-session with Artem. This was hard work!
We started at about 14:00 and wanted to add some unittests for a class Artem wrote. I already reviewed his code, so I knew it quite good. By the way, one result of the review was "where are the unittests?" - so I maneuvered myself in this position. Anyway, good quality-code needs some sacrifices *heroic*
I took the keyboard and Artem the mouse... No, just kidding. He was sitting beside me keeping an eye on my cursor. So we began with the first function and let Visual Studio build a test-stub and an accessor (yes, I love that!) for us. We discussed what to test for this method and I began to write code for it - easy. Not long afterwards, we found the first inconsistency which wasn't visible in the review and in the productive code either. Fixed it and continued. This went on and on, one unittest after the other. We discussed the relevant parts of the code and enhanced a few places.
But as time goes by, I noticed how unconcentrated I was and that I made more and more typos. What a luck that we almost had all unittests we longed for. So we created the rest and finished that session. It was 16:00 and I was exhausted like never before when writing some code.
I must say, that this was hard, but it was worth for it. You can't get such a tight feedback-loop when doing reviews and you can't produce code just by discussing it in a normal meeting. So, pair programming is great but exhausting!

Clean Code

I just read the german edition of Clean Code from Robert C. Martin. The introduction made me love this book immediately by a simple drawing. There are two things I’ll take along from this book:

1) For each function f let f.LinesCount < 20

2) Comments are evil

These two rules need some clarifications of course. In fact, the demand to keep functions below 20 lines is a result of another rule: Don’t mess with abstraction-levels. If you see a function with more than 20 lines, this should be a good indication for a mix of different levels of abstraction.

The second rule points in the opposite direction of what I’ve learned at university: one tutor said to me that there should be more lines of comment than lines of code, so that another person could easily understand what you wanted to say. Today with C# and Java, this isn’t true anymore. The code itself became the comment. No more cyptic function-calls or Hungarian notation but intuitive named functions and variables telling what they’re doing.

Although I’m still feeling awkward without any function-header-comment, I slowly try to reduce unnecessary comments like this:

‘ iterate through all alarms
For Each a In m_colAlarms

And transform it into this

For Each alarm In _alarms

The difference is not big but the sum of all changes generates a nice picture.

So, the first dozen chapters are truly inspirational. After that, the book takes a look on bigger examples how to refactor whole classes. This is done step-by-step and after some steps, it’s getting boring. My advice: just read the first 12 chapters and start coding!

What a wonderful world

Recently I read a lot about the M-V-VM pattern and I wanted to try it out. So I started small with a WPF-application implementing a listview showing some events related to sleep medicine. I’ve created a simple model with a collection of various data. This model is the data-provider for a ViewModel which is databind to this view:

So far, so good; worked pretty easy. I then read something about the graphical ability of WPF, which should be very bad in displaying a lot of graphical objects. Again, I wrote a sleep-medical-application, but this time showing some random biosignals with events:

The problem here was the very long signal build of thousands of Polyline-segments. And indeed, this brought WPF to its knees. You can’t show some signals with the pure use of WPF-technology. I instead used GDI+ inside of a Canvas-element for the signal and the VirtualCanvas-technique for the events (read here about it). With that I managed to preserve databinding on the modifiable objects (the events): you can move them around and change their duration by dragging their border.

In a second step, I combined the model of the ListView with that of the biosignals and injected it into its ViewModel. Of course, every ViewModel is being unittested – what a wonderful world:

LoC and FxCop

I was curious on how many lines of codes we are working. There were only nebulous guesses of around 100.000 locs – but no official ones. So I googled a bit and found CLOC, a counting tool for lines of code. I ran it on my ActiveX- and DotNet-folder and it calculated:

·         80,000 loc VB.NET and 40,000 lines of comments in VB.NET

·         410,000 loc VB6 and 310,000 lines of comments in VB6

·         20,000 loc C++ and 5,000 lines of comments in C++

The good number of comments in VB6 might result of the huge header we gave each file and a rather big header for each method. In VB.NET this is more dense.

 

Another tool on my way to a great developer is FxCop. This program can examine a dll if certain rules are broken. You can select and deselect single rules or whole rule-sections. The rules are made by Microsoft but you can build your own rules and add them. When you run the program, it lists all violation and you can either try to fix them or if you disagree, you can exclude them with a comment.