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.

Prism is witchery

For my newest project, I wanted to use Prism to guide us writing a decoupled WPF-application. I started investigating it and was quickly amazed about the module-approach. In our former WinForms-project, we didn't used a framework to guide us building a nice architecture, but developed our own clsForms-class (at that time even with hungarian notation *awkward*) and used even IPC to communicate across process-boundaries.

I began writing the framework for the new project with heavy guidance of Prism (RegionManager, EventAggregator, DelegateCommands and CommandBehaviors). In the end, we had some modules building a master-detail view with database-access. This was the time when other coworkers started to complain that the architecture is way too complex for such a small application as it is today. We started to discuss how to simplify it and at the end we had a draw in our opinions: some people wanted to keep everything (me included *g*) and some wanted to strip it down to remove Prism, to abolish dependency injection and the IoC container Unity. Our boss had to decide it and spoke to us: "thou shalt remove Prism but keep DI for a better TDD").

The top-3 reasons why Prism is evil for us* are:
(*) i.e. not for me

3) Using the app.config for setting up the modules is error-prone and not type-safe.
I told them that we could do it in code also but somehow I got not heared :/

2) Debugging is too hard.
When you get an exception in a view which will be inserted into a region, you got this Prism-Exception telling you that there was a problem resolving this module for that region. I learned here that you cannot expect everyone to look at inner-exceptions (admittedly mostly at level >5)...

1) Top reason why Prism is evil: it is too intransparent how the regions get filled (witchery *hooo*).
What can I say? For sure, you have to look at the good documentation at their homepage to get the hang of it. Else it's witchery, yes.

So we burned it and removed it from our solution with 10+ modules and about 10,000 loc. We needed 5 hours to remove the lightweight Prism-sections completely and substitute it with our own RegionManager-approach (which is basically the same) and with our own DelegateCommand-approach (wich is basically the same) and without the help for commanding of Prism (we now use code-behind to execute the commands). Ah yes, and we copied the EventAggregator out of the Prism-sourcecode and use it now, because our approach would be basically the same...

Everyone feels better now that the evil is being distroyed.

What was the problem? We're using a whole bunch of new technologies and paradigms in this project (WPF is new to us, as Entity framework, WCF, Prism, TestDrivenDevelopment and dependency injection). This means a lot to learn and sooner or later, your retentiveness is exhausted. This meant to sacrifice a pawn, in this case it was Prism.

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:

MSDN magazine and Unity

οἶδα οὐκ εἰδώς, oída ouk eidós!

Thank you wikipedia for helping me with my greek. In the last few weeks, I learned so much while considering this and that for the new architecture; my programming-knowledge seems to duplicate with every line I read. Was I completely dazzled all the time?

Well, I'm reading the MSDN magazine now (in fact, I'm trying to manage to read a tiny part of it) to keep me up to date at least with the MS-world. By the way, in the february-issue, there is a great article about MVVM by Josh Smith.

One cool new think I learned is IoC of famous Martin Fowler. So I looked at Unity, a relatively new dependency injection container. Imho a great way for spreading logging and parts of our MCC_Aufzeichnung. Works fine with MVVM as Jason Dolinger demonstrates. This video was kind of an eye-opener in the matter of WPF for me.