A Refactoring Journey

I have spent a week heavy on the refactoring side. It’s been a while since I spent so much time on a “virgin” codebase, growing it test first. And I want to share with you, guys and girls, a piece of refactoring that ended up being pretty cool.

Triangulate, always triangulate

I was happily developing one class at a time. But since the first class I felt an tingle. Like those lines drawn from Spiderman’s face. spidersenseThe tingle became an itch when, while developing a second class, I was doing pretty much the same thing. And the itch became a rash when, for the third time I did the same.
The thing in question, was performing one of two actions depending of the presence or absence of value of a pair of variables.

But everyone knows that you do not scratch when you have a rash. Right? RIGHT!? One does not go blindly hurting himself. In the case of code, one  does not endeavor refactoring quests without having a set of tests that make you confident that things will be as they were before and no new defects have been introduced. Right? RIGHT!?
In my case, I was going tests first, so I had a battery of tests for the classes that provoked the rash.

Moral of the story is: one should put up with a tingle, could put up with an itch and must act with the rash. Bringing “the big guns” for a tingle does not make sense and can be counter-productive.

Primitives are evil, value objects FTW

In reality they are not evil. They are very useful indeed, but their main feature (its generality) is usually their biggest disadvantage.
There are million of examples when primitive types are used for sheer laziness but, in reality, a custom value object communicates the concept way better.
For example: can one really perform all operations of a string on a conventional name? does that number represent meters or inches? probabilities bigger than one? These and other aberrations can (and do) happen when the concept is represented with a primitive type (a string, an integer or a floating point number).

In the case that occupies us, the two operations have a value object as a subject, although the value objects (which happen to be nullables) are created out of Nullable<T> primitive types.

public struct ValueObject_A
{
    public ValueObject_A(int a) : this()
    {
        Value = a;
    }
    public int Value { get; private set; }
}
public struct ValueObject_B
{
    public ValueObject_B(decimal b) : this()
    {
        Value = b;
    }
    public decimal Value { get; private set; }
}

The Choice

This choice value object have multiple but cohesive responsibilities:

  1. ensure that only one of the possibilities is active (not two, not zero)
  2. build the value object out of the valued primitive and nullify the other
  3. perform one out of two actions
public class AorB
{
    private ValueObject_A? A { get; set; }
    private ValueObject_B? B { get; set; }
    public AorB(int? a, decimal? b)
    {
        ensureOnlyOne(a, b);
        A = a.HasValue ? new ValueObject_A(a.Value) : default(ValueObject_A?);
        B = b.HasValue ? new ValueObject_B(b.Value) : default(ValueObject_B?);
    }
    private void ensureOnlyOne(int? a, decimal? b)
    {
        if ((a.HasValue && b.HasValue) || (!a.HasValue && !b.HasValue))
        {
            throw new NotSupportedException("Either A or B");
        }
    }
    public string Do(Func<ValueObject_A, string> actionWithA, Func<ValueObject_B, string> actionWithB)
    {
        return A.HasValue ? actionWithA(A.Value) : actionWithB(B.GetValueOrDefault());
    }
    public static AorB OnlyA(int a)
    {
        return new AorB(a, default(decimal?));
    }
    public static AorB OnlyB(decimal b)
    {
        return new AorB(default(int?), b);
    }
}

He chose… poorly

Or we could, instead.

One abstraction introduced, one abstraction that can be tested in a fraction of the time it takes to deploy the thing, fire up the complete application, exercise the feature, see it burn because you suck at boolean logic, attach the debugger,… you get the picture, right? RIGHT!?
Let’s check that the correct function is executed according to the presence of the value, and that the other function is not executed.

One can walk the anonymous method path, but will find it noisy with all those curly braces:

[Test]
public void Do_UseAnonymousMethods_ABitNoisy()
{
    var subject = AorB.OnlyA(3);
    Func<ValueObject_B, string> notExecuted = b =>
    {
        Assert.Fail("must not execute");
        return string.Empty;
    };
    bool wasExecuted = false;
    Func<ValueObject_A, string> executed = a =>
    {
        Assert.That(a.Value, Is.EqualTo(3));
        wasExecuted = true;
        return string.Empty;
    };
    subject.Do(executed, notExecuted);
    Assert.That(wasExecuted, Is.True);
}

One can step to remove noise by using methods as the functions, but we can see duplication coming:

[Test]
public void Do_UseMethodForNotExecutedFunc_LeadsToDuplication()
{
    var subject = AorB.OnlyB(3.5m);
    bool wasExecuted = false;
    Func<ValueObject_B, string> executed = a =>
    {
        Assert.That(a.Value, Is.EqualTo(3.5m));
        wasExecuted = true;
        return string.Empty;
    };
    subject.Do(notExecuted, executed);
    Assert.That(wasExecuted, Is.True);
}
private string notExecuted(ValueObject_A a)
{
    Assert.Fail("must not execute");
    return string.Empty;
}
// same old, same old
private string notExecuted(ValueObject_B b)
{
    Assert.Fail("must not execute");
    return string.Empty;
}

Duplication? Couldn’t that type of duplication be solved by using generics? I heard that they are not only for collections Winking smile

[Test]
public void Do_GenericForTheRescue_RemovesDuplication()
{
    var subject = AorB.OnlyA(3);
    bool wasExecuted = false;
    Func<ValueObject_A, string> executed = a =>
    {
        Assert.That(a.Value, Is.EqualTo(3));
        wasExecuted = true;
        return string.Empty;
    };
    subject.Do(executed, delegate(ValueObject_B b) { return notExecuted(b); });
    Assert.That(wasExecuted, Is.True);
}
private string notExecuted<T>(T aOrB)
{
    Assert.Fail("must not execute");
    return string.Empty;
}

But… wasn’t it about noise? Are you kidding me? An anonymous delegate syntax in 2012? Yeah, but if I show you the goodies from the beginning where’s the fun?

[Test]
public void Do_WithTypeInference_SlimAsItCanBe()
{
    var subject = AorB.OnlyB(3.5m);
    bool wasExecuted = false;
    Func<ValueObject_B, string> executed = a =>
    {
        Assert.That(a.Value, Is.EqualTo(3.5m));
        wasExecuted = true;
        return string.Empty;
    };
    subject.Do(notExecuted, executed);
    Assert.That(wasExecuted, Is.True);
}
private string notExecuted<T>(T aOrB)
{
    Assert.Fail("must not execute");
    return string.Empty;
}

And here we are, no duplication for the notExecuted() function and syntax clean as a whistle: subject.Do(notExecuted, executed). It may seem not much, but do I have to remind you that refactoring is an art of evolution without revolution?

The journey

  1. tingly code
  2. itchy code
  3. rash-y code
  4. rash-y code with tests
  5. introduce value objects A and B
  6. new abstraction AorB
  7. test abstraction
  8. use generics and type inference to test simple abstraction
  9. be amazed
  10. replace rashy-code with tested abstraction
  11. green lights everywhere
  12. party!

Kategorier: Udvikling

Tagged as:

Skriv et svar

Udfyld dine oplysninger nedenfor eller klik på et ikon for at logge ind:

WordPress.com Logo

Du kommenterer med din WordPress.com konto. Log Out / Skift )

Twitter picture

Du kommenterer med din Twitter konto. Log Out / Skift )

Facebook photo

Du kommenterer med din Facebook konto. Log Out / Skift )

Google+ photo

Du kommenterer med din Google+ konto. Log Out / Skift )

Connecting to %s