Continuous refactoring in its natural habitat

So over the last three weeks or so, after the kids went to bed, I've been working on some new features for Fluent Assertions . While doing so I went off track several times in an attempt to improve several API's and internal designs that didn't felt quite right. Since this thought process is representative for the way I approach professional software development and my blog is about continuous improvement I decided to try converting my brain's thought process in a post.
Before I go into the deep, let me share some context for the problem at hand. Fluent Assertions (let's call it FA from now) has an API to compare two object graphs which internally uses a collection of implementations of the IEquivalencyStep interface. As part of the next release, I wanted to allow people to directly affect the API's behavior by adding, removing or replacing steps with their own. Before that change, the EquivalencyValidater class had a method GetSteps to provide it with the out-of-the-box equivalency steps.

private IEnumerable<IEquivalencyStep> GetSteps()
{
    yield return new TryConversionEquivalencyStep();
    yield return new ReferenceEqualityEquivalencyStep();
    yield return new RunAllUserStepsEquivalencyStep();
    yield return new GenericDictionaryEquivalencyStep();
    yield return new DictionaryEquivalencyStep();
    yield return new GenericEnumerableEquivalencyStep();
    yield return new EnumerableEquivalencyStep();
    yield return new StringEqualityEquivalencyStep();
    yield return new SystemTypeEquivalencyStep();
    yield return new EnumEqualityStep();
    yield return new StructuralEqualityEquivalencyStep();
    yield return new SimpleEqualityEquivalencyStep();
}

So I started to move those defaults into a new static AssertionOptions class. Yes, I know, it's static, but it's supposed to be global and should affect all usages of FA.

public static class AssertionOptions
{
    private static List<IEquivalencyStep> steps = new List<IEquivalencyStep>();

    static AssertionOptions()
    {
        steps.AddRange(GetDefaultSteps());
    }

    private static IEnumerable<IEquivalencyStep> GetDefaultSteps()
    {
        yield return new TryConversionEquivalencyStep();
        yield return new ReferenceEqualityEquivalencyStep();
        // …left out for brevity
    }
}

Practicing object-oriented design

But then my obsession about maintainable code kicked in. Why would I overload the AssertionOptions class with the responsibilities and knowledge on where to insert new steps in relation to the built-in steps? So let's apply rule 4 of Object Calisthenics which is also known as First Class Collections:

Any class that contains a collection should contain no other member variables. If you have a set of elements and want to manipulate them, create a class that is dedicated for this set.

I cannot stress this enough. Whenever your class contains multiple private fields, please consider extracting those in dedicate collection classes or value types. It might feel as unnecessary refactoring, but it's really going to make your code mode object-oriented and maintainable. Regardless, after refactoring all this logic ended up in a new EquivalencyStepCollection that is used like this:

public static class AssertionOptions
{
    private static EquivalencyAssertionOptions defaults = new EquivalencyAssertionOptions();

    static AssertionOptions()
    {
        EquivalencySteps = new EquivalencyStepCollection(GetDefaultSteps());
    }

    public static EquivalencyStepCollection EquivalencySteps { get; private set; }
}

The collection class really behaves as a collection and implements, at a minimum, IEnumerable:
public class EquivalencyStepCollection : IEnumerable<IEquivalencyStep>

{
    private readonly List<IEquivalencyStep> steps = new List<IEquivalencyStep>();

    public EquivalencyStepCollection(IEnumerable<IEquivalencyStep> defaultSteps)
    {
        steps.AddRange(defaultSteps);
    }

    public IEnumerator<IEquivalencyStep> GetEnumerator()
    {
        return steps.GetEnumerator();
    }

    IEnumerator IEnumerable.GetEnumerator()
    {
        return GetEnumerator();
    }
}

I didn't mention it before, but I wouldn't be taken myself seriously if I would not be practicing Test Driven Development. So one of those tests involves specifying the behavior of adding a step and making sure it ends up just before the built-in step that does a simple comparison using Object.Equals:

[TestClass]
public class When_appending_a_step : Given_temporary_equivalency_steps
{
    public When_appending_a_step()
    {
        When(() =>
        {
            Steps.Add<MyEquivalencyStep>();
        });
    }

    [TestMethod]
    public void Then_it_should_precede_the_final_builtin_step()
    {
        IEquivalencyStep builtinStep = Steps.LastOrDefault(s => s is SimpleEqualityEquivalencyStep);
        IEquivalencyStep addedStep = Steps.LastOrDefault(s => s is MyEquivalencyStep);
        int builtinStepIndex = Steps.LastIndexOf(builtinStep);
        int addedStepIndex = Steps.LastIndexOf(addedStep);

        addedStepIndex.Should().Be(builtinStepIndex - 1);
    }
}

Intention-revealing unit tests

Did you notice that I'm hiding some of the complexities needed to reset the static AssertionOptions class in a base-class? I'm not in favor of test base-classes, especially because they tend to get misused pretty quickly. But with the help of Chill, a project by Erwin van der Valk, I decided to use one anyhow, simply because it helps clarify the intend of my test. I think it was Jeremy D. Miller that once said "If it's not important for the unit test, it's very important not to show it" and clearing up after a test is not important to understand the test. This is what the base-class looks like. Notice Chill's GivenWhenThen class.

public class Given_temporary_equivalency_steps : GivenWhenThen
{
    protected override void Dispose(bool disposing)
    {
        Steps.Reset();
        base.Dispose(disposing);
    }

    protected static EquivalencyStepCollection Steps
    {
        get { return AssertionOptions.EquivalencySteps; }
    }
}

Did you also notice the implementation of the Then_it_should_precede_the_final_builtin_step? It's basically a copy of the internal implementation of the Add method, so I hardly think it is helping on the intention revealing side of my story. I'm sure you'll agree. This is where my code quality obsession kicked in again, so I decided to extend FA with some specialized extension methods that would help me making those tests a bit more intention revealing.

But wait! I surely don't want to pollute my current changes with even more refactoring, would I? No, I definitely prefer small commits and a clean and tidy commit history. But switching to another branch without committing those half-finished changes is not going to allow me to start with a clean slate. Sure, I could stash my changes, but that requires me to think of some unique name. And yes, I do have a second clone somewhere on my SSD, but I'd rather create a temporary commit that I can use to rebase on those new assertions at a later point. Well, that's just what Phil Haacked's git save and git undo aliases do.


After installing those aliases and executing git save from your favorite git bash or PowerShell console (don't forget Posh-Git if you do) will take the local changes and commit those as a local commit named SAVEPOINT. Now I can safely switch to a new branch (git cob does just that) and work on those extensions.


One of the first assertion methods I implemented was the collection.Should().StartWith() method. After the first spec representing the happy path it looked like this:

public AndConstraint<TAssertions> StartWith(object element, string because = "", params object[] becauseArgs)
{
        object first = Subject.Cast<object>().FirstOrDefault();

        Execute.Assertion
            .ForCondition(first.IsSameOrEqualTo(element))
            .BecauseOf(because, becauseArgs)
            .FailWith("Expected {context:collection} to start with {0}{reason}, but found {1}.", element, first);
            return new AndConstraint<TAssertions>((TAssertions) this);
}

Finding a better assertion API

But after finishing all the other paths as part of me practicing TDD, it ended up like this.

public AndConstraint<TAssertions> StartWith(object element, string because = "", params object[] becauseArgs)
{
    bool succeeded = Execute.Assertion
        .ForCondition(!ReferenceEquals(Subject, null))
        .BecauseOf(because, becauseArgs)
        .FailWith("Expected {context:collection} to start with {0}{reason}, but the collection is {1}.", element, null);

    if (succeeded)
    {
        succeeded = Execute.Assertion
            .ForCondition(Subject.Cast<object>().Any())
            .BecauseOf(because, becauseArgs)
            .FailWith("Expected {context:collection} to start with {0}{reason}, but the collection is empty.", element);
    }

    if (succeeded)
    {
        object first = Subject.Cast<object>().FirstOrDefault();

        Execute.Assertion
            .ForCondition(first.IsSameOrEqualTo(element))
            .BecauseOf(because, becauseArgs)
            .FailWith("Expected {context:collection} to start with {0}{reason}, but found {1}.", element, first);
    }

    return new AndConstraint<TAssertions>((TAssertions) this);
}

This implementation is quite representative for most of the other extension methods in FA, but somehow it didn't feel right. I was planning to include EndWidth and HaveElementPreceding as well, but I wasn't looking forward to more of these monstrosities. In particular the constructs with the succeeded variable don't help understanding the code. You might expect FailWith to throw some kind of exception when the condition is not met, and usual it does. But the structural equivalency API uses an AssertionScope to collect all assertion failures and will throw them as one failure at the end. In fact, anybody can build extensions to FA and use the AssertionScope in some more advanced scenarios.

Anyway, I decided to commit those changes and give myself a couple of days to come up with a better approach. I already knew I was going to create some kind of fluent API, but I needed a bit of time to chew on it. This is what I ended up with:

public AndConstraint<TAssertions> StartWith(object element, string because = "", params object[] becauseArgs)
{
    Execute.Assertion
        .BecauseOf(because, becauseArgs)
        .WithExpectation("Expected {context:collection} to start with {0}{reason}, ", element)
        .ForCondition(!ReferenceEquals(Subject, null))
        .FailWith("but the collection is {0}.", (object)null)
        .Then
        .Given(() => Subject.Cast<object>())
        .ForCondition(subject => subject.Any())
        .FailWith("but the collection is empty.")
        .Then
        .Given(objects => objects.FirstOrDefault())
        .ForCondition(first => first.IsSameOrEqualTo(element))
        .FailWith("but found {0}.", first => first);

    return new AndConstraint<TAssertions>((TAssertions) this);
}

What's important to know is that the Given and Then members are not even invoked if the previous condition was not met. Granted, it's more than my typical maximum of 7 statements, but it allowed me to get rid of those intermediate boolean variables and prevent repeating the expectation message. And with that, implementing the other extension methods became pretty easy.

Getting back on track

So, after committing those changes back to develop, my main development branch (I'm using the Gitflow branching strategy), it was time to back-track to the global AssertionOptions API I began this post with. I started that work on a separate branch which head now pointed to the temporary commit I created using git save. To get my working directory to the state it was before I side-tracked, but including the new extension methods was just a matter of doing a git pull develop --rebase to replay the changes on my feature branch on top of develop, followed by git undo to restore my work-in-progress from that temporary commit. I don't understand how I managed to get anything done without those aliases.

Anyway, this is one of those final unit tests.

public class When_appending_a_step : Given_temporary_equivalency_steps
{
    public When_appending_a_step()
    {
        When(() =>
        {
            Steps.Add<MyEquivalencyStep>();
        });
    }

    [TestMethod]
    public void Then_it_should_precede_the_final_builtin_step()
    {
        var equivalencyStep = Steps.LastOrDefault(s => s is SimpleEqualityEquivalencyStep);
        var subjectStep = Steps.LastOrDefault(s => s is MyEquivalencyStep);

        Steps.Should().HaveElementPreceding(equivalencyStep, subjectStep);
    }
}

I'm doing all of this in my private hours so side-tracking from my original goal so much is not a typical situation for me either. I fully realize that this is usually not an option in real projects. Regardless, if you ask me, you should strive for continuous improvement every single day. One practical way of tracking these kinds of refactorings is to create check lists on Github or in OneNote. Another method I'm experimenting with is to insert dedicated comments to mark code as smelly or to suggest possible refactoring ideas. You can read more about this workflow in the article Natural Cause of Refactoring. All being well, whatever you do, please never forget The Boy Scout Rule:

Always leave the campground cleaner than you found it

So what do you do to continuously improve your code base? Let me know by commenting below or tweeting me at @ddoomen.