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.

CodeRush Xpress

Some time ago, I tried ReSharper, but wasn't totally convinced to spend > 100 Euro - so I forgot about it. As I'm doing more and more refactorings (you know, I'm on my way...), I really felt that there could be tools to help me. I read some forums and found CodeRush Xpress. Just have a look at the introductory video here and you want to work immediately with it.
I discovered that tool the day before I went on a 1-week-no-pc-holiday-trip. As I came back, I was eager to try it and it held all my demands.
I'm using + + <+ / -> (Selection increase / decrease), the camel-case navigation and on a variable (show appearance). And of course I'm using + <ö> (german layout) to call the refactorings. It's nice how you get a visual feedback for an upcoming refactoring:

There's so much so discover for me. There's also a kind-of mod-community, because the API is open (DXCore) to extend. Great tool!

How to build a collection of Interfaces

Here’s the problem: we have some files and some devices which operate with signals. As a signal is one of our main objects, we want to return a collection of signals when opening a file or beginning a communication with a device:

But of course, we want to handle each signal the same, so a signal from an EDF-file should be from the same class as a signal from a device. We could define a signal-class in the Main-library like this:

But this constructs a circular reference. We could create a converter for each special signal and build a collection in the main-library of all signals:

Ouch – ugly. For every signal-type we need one converter. What happens when DeviceSignal changes? Hopefully, we update the corresponding DeviceSignalConverter... Here’s my solution:

We implement all interfaces a signal could be loaded / imported from in the main-signal-class and use generics to simulate a factory. A EDF-dll could have:

  public interface ISignal {
      int SamplesPerRecord { get; set; }
  }

  public class SignalReader where T : ISignal, new() {
      public override ICollection GetSignals() {
          List signals = new List();
          var signal = new T();
          signal.SamplesPerRecord = 3;
          signals.Add(signal);
          return signals;
      }
  }

The signal in the main-library implements that interface:

   public class Signal : ISignal {
       public int SamplesPerRecord { get; set; }
   }

We can now call the SignalReader and it returns us a collection of signals we can use:

   public ICollection GetSignals() {
       SignalReader reader = new SignalReader();
       return reader.GetSignals();
   }

We can implement further signal-readers just by implementing the necessary interfaces into the main-signal-class. In fact, for each reader we created from Signal inherited classes implementing the interface the reader defines. With that construct, we can work without circular references and get a collection of objects we can work with.

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:

C#-For and VB.NET-For

Yesterday I wondered why on earth my simple VB.NET-For-loop produced an error:

Dim collection As New List(Of Integer)
collection.Add(1)
collection.Add(2)
collection.Add(3)
collection.Add(4)
collection.Add(5)

For i As Integer = 0 To collection.Count - 1
  Console.WriteLine(collection(i))

  If collection(i) = 2 Then
    collection.RemoveAt(i)
  End If
Next

I’m coming from a Java / C#-background, so I tried the same loop in C#:

var collection = new List<int>();
collection.Add(1);
collection.Add(2);
collection.Add(3);
collection.Add(4);
collection.Add(5);

for (int i = 0; i < collection.Count; i++) {
  Console.WriteLine(collection[i]);

  if (collection[i] == 2) {
    collection.RemoveAt(i);
  }
}

Et voilá, the result is what I wanted. So I made my discovery of the day: C#-For isn’t VB.NET-For.

In the cold light of day and using the debugger, it seems obvious that the VB.NET-For doesn’t check the condition on every iteration. It just duplicates the loop as often as it is stated in the For-statement.

The C#-construct is smarter as it checks in every iteration if the condition is true. But if it’s true that this is the difference between the two Fors, then the C#-loop must be slower. So, let’s check it out:

Dim start = DateTime.Now

For i As Integer = 0 To 1000000000
  ' nothing
Next

Console.WriteLine(Now.Subtract(start).ToString())

Vs.

var start = DateTime.Now;

for (int i = 0; i < 1000000000; i++) {
  // nothing
}

Console.WriteLine(DateTime.Now.Subtract(start).ToString());

And indeed, the VB.NET-For-loop wins on my system with 3.734s to 4.687s!

Thread-Threat

At work, I'm trying to get a software-feature faster by introducing multithreading. We import data from a device and convert it into our format. This is done sequentially at the moment, first importing, then converting. First tests have shown that we can get a benefit of about 30% if we do that in parallel. So I developed a wonderful producer-consumer-queue with a producer-thread and a consumer-thread which gets its data from the producer-thread. So far, so good: all unit-tests with that queue ran successfully.

However, as soon as I plugged that code into the import-conversion-piece to make it multithreaded, I got System.Runtime.InteropServices.COMExceptions with HRESULTs of -2147467259 (RPC_E_SERVERCALL_RETRYLATER, Unspecified error) or -2147417856 (RPC_E_SYS_CALL_FAILED, System call failed) - (here is a list with automation errors). These errors occured randomly on different parts of the code. But all code-parts had in common that they're calling a VB6-COM-object (well, it's a COMException - what should I expect...).

Some googling brought me similarities with Office-automation (see here and here for example). My picture about that problem became clearer and clearer and I sketched that diagram to understand it:

Thread A and B are accessing the same COM-object. This access is provided via a RCW (runtime callable wrapper). There is only one RCW for all accesses to that COM-object (read the great article series "Beyond (COM) Add Reference: Has Anyone Seen the Bridge?" of Sam Gentile for a deeper dive into COM-interop). This means multithreading and COM-interop is no fun.

What about protecting the access to that COM-interop with a lock? I designed a little construct so that I won't forget to protect any calls to that COM-interop:

The wrapper for the COM-interop, ThreadSafeComInterop, offers the COM-interop via a Lock-class. Each time you want to access the COM-object, you acquire the lock. With that, a Monitor enters the lock and exits it not until the lock is disposed. It's neat with the using-construct:

Using lock = _interop.Lock
' do something with lock.ComInterop
End Using

See that sequence-diagram for further information:

The provider creates the ThreadSafeComInterop by passing the COM-interop to the constructor. The recipient can call the Lock via the using-construct and access the COM-interop via lock.ComInterop. By creating the ThreadSafeComInteropLock, a Monitor.Enter is established on a static lock-object. When finished using the COM-object, the Lock is being disposed and Monitor.Exit is being called to allow other components to access the COM-object.

This sounds great and tests are showing that the exception-rate is degreasing from 10 tries - 9 exceptions to 10 tries - 1 exception. But unfortunately, this exception is one exception too much. Reading "Lessons Learned Automating Excel from .NET" finally convinced me to write the needed parts of the COM-component in .NET.

BendingStackPanel

I just finished a nice little tool for our Message-module: Message.dll for dummies.

On the right is a ListView with various components of which the trace-level can be altered. In the center of the dialog are 3 Buttons to vary the debug-mode. These buttons are on a StackPanel with a custom-made property "Bend" and overrides to Measure and Arrange. You can bend the StackPanel up to 180 degrees. Here is a picture with more buttons and an ellipse (any UIElement can be added):

The slider and the TextBlock binds to the BendingStackPanels Bend-property so that you can interactively bend and unbend the panel - looks really cool :) I was amazed how straightforward the development was: it took me just a few hours! Here is the envelope of ideas - nice, eh? ;)

The whole bending-stuff is just class-10 (yes, I looked it up...) sine- and cosine-trigonometry. You can have a look at the code here.

TimeSpan and EDF

I noticed two things while playing with EDF. TimeSpan.TotalSeconds is inaccurate on some computers. Probably the value is calculated by using the ticks-property. Unfortunately, this leads to a double which can have the wrong number. I made a t = TimeSpan.FromSeconds(3.2) and got t.TotalSeconds = 3.1999999999997. When using that in a calculation and casting the result to an Integer, you can get a wrong number - like me...

Speaking of wrong numbers brings me to my 2-day-EDF-problem. I desperately tried to create a sup-file made of dummy-EDF-files. The code is actually quite simple: new clsAufzeichnung, Meas_Sections.Add(dummy-EDF-file), SaveAufzeichnung, done. This works when you have set the HeaderLength of the EDF-file - unlike me...

Pack:=1 and ASP.NET: The beginning

Uff, just fighted for two days of getting that byte-array into a structure. There were always strange values starting from a specific property. After a few hours, I found out that there was a off by 1 byte error: if I shift every byte back one byte, I will reach the right result. Code tells more than 1000 words:

Structure Test
  Dim a As Byte
  Dim b As Int16
  Dim c as Byte
End Structure

Dim handle As GCHandle = GCHandle.Alloc(byteArray, _
                                      GCHandleType.Pinned)
Dim t As New Test
t = CType(Marshal.PtrToStructure(_
                         handle.AddrOfPinnedObject(), _
                         GetType(Test)), Test)

This will give you false values for b and c. This is why: "The default behaviour in the framework is for values in the structure to be aligned at 4-byte boundaries, so values start at FieldOffset 0,4,8,12 ..." (see forum). To avoid this, use as a structure-attribute and everything's fine now :)

Another bad book read: Programmieren mit ASP.NET AJAX. The first chapter is good - everything else just confusing. No good examples. Maybe I didn't get the point - I haven't done anything in ASP.NET yet. I'm watching now and the first 6 videos were fine :)

Ah, I forgot to mention the free first chapter of Professional ASP.NET MVC 1.0 from Rob Conery, Scott Hanselman, Phil Haack and Scott "ScottGu" Guthrie - famous bloggers writing a book. I'm curious to read it :)