Object Calisthenics #3: Bringing Order to Chaos by Yoan

Object Calisthenics, a set of programming exercises designed to improve code quality and object-oriented design, can be a powerful tool when refactoring existing codebases.

To see how they work, Pierre and I decided to do the exercise on our own, without influencing each other.

If you haven't read it, here's the link to the explanation of the exercise to which this article is a response:

This article chronicles my experience applying these principles to a Lord of the Rings-themed project, highlighting the steps taken and the reasoning behind each decision.

Setting the Scene

I started with FellowshipOfTheRingService that was, well, less than magical. It was riddled with code smells and violations of object calisthenics principles.

Here's a snippet of what I was dealing with:

public class FellowshipOfTheRingService
{
    // First Class Collections: A missed opportunity
    private List<Character> members = new List<Character>();

    public void AddMember(Character character)
    {
        // Useless check: unnecessary null check
        if (character == null)
        {
            throw new ArgumentNullException(nameof(character), "Character cannot be null.");
        }
        // Don't Abbreviate: unclear abbreviation usage
        // What does 'N' represent? The name, as hinted by the exception message...
        else if (string.IsNullOrWhiteSpace(character.N))
        {
            throw new ArgumentException("Character must have a name.");
        }
        // Don't Use the ELSE Keyword: excessive usage throughout the method
        else if (string.IsNullOrWhiteSpace(character.R))
        {
            throw new ArgumentException("Character must have a race.");
        }
        // Excessive Null Checks: scattered throughout the method
        else if (character.W == null)
        {
            throw new ArgumentException("Character must have a weapon.");
        }
        // One Dot Per Line: violated in multiple instances
        else if (string.IsNullOrWhiteSpace(character.W.Name))
        {
            throw new ArgumentException("A weapon must have a name.");
        }
        else if (character.W.Damage <= 0)
        {
            throw new ArgumentException("A weapon must have a damage level.");
        }
        else
        {
            // Only One Level of Indentation Per Method: broken here
            bool exists = false;
            foreach (var member in members)
            {
                // No Getters/Setters/Properties: direct field access
                if (member.N == character.N)
                {
                    exists = true;
                    break;
                }
            }

            if (exists)
            {
                // Excessive Exceptions: scattered throughout the code
                throw new InvalidOperationException(
                    "A character with the same name already exists in the fellowship.");
            }
            else
            {
                members.Add(character);
            }
        }
    }
    ...
}

Our mission:

To refactor it into something Gandalf himself would approve of.

The Path to More Readable / Maintainable Code

1. Characterization Tests: Our Ring of Protection

Before I dared touch the existing code, I created characterization tests using Verify:

[Fact]
public Task Program_Should_Run_Same()
{
    App.Run();
    return Verify(_console.ToString());
}

Verify is a snapshot testing tool that captures the output of your code and compares it against a known good state. When you first run the test, it creates a snapshot file. On subsequent runs, it compares the current output to this snapshot. This helps ensure that your refactoring doesn't change the existing behavior.

These tests acted like the One Ring, preserving the existing behavior of our code against unintended changes.

2. Surface Refactoring: Cleaning the Shire

I started with some quick wins, using my IDE to perform surface refactoring. This included removing redundant code, introducing constants, and converting some logic to more readable LINQ expressions

3. Don't Abbreviate: Speaking in the Common Tongue

I tackled the cryptic abbreviations in the code. N became Name, R became Race, and so on. This made the code more readable, as if I had translated it from Elvish to the Common Tongue.

public sealed class Character
{
    public string Name { get; set; }
    public string Race { get; set; }
    public Weapon Weapon { get; set; }
    public string CurrentLocation { get; set; } = "Shire";
}

4. Wrap Primitives and Strings: Forging New Types

Here's where I employed Test-Driven Development (T.D.D) and the Sprout technique. The Sprout technique involves creating new code separately from the existing code, then gradually integrating it. This allows us to develop and test new functionality without immediately impacting the existing system.

I created new types on the side of the production code, starting with tests (here a property one):

// Express how to generate valid names
private static Arbitrary<string> NonEmptyString() => Arb.Default.String()
    .Filter(str => !string.IsNullOrWhiteSpace(str));

[Fact]
public void Parse_A_Valid_Name()
    => Prop.ForAll(
        NonEmptyString(),
        validName => Name.Parse(validName).ToString() == validName)
        .QuickCheckThrowOnFailure();

And implemented the Name class:

public sealed class Name
{
    private readonly string _value;
    private Name(string value) => _value = value;
    // Parse is the only way to instantiate it (Factory Method)
    public static Name Parse(string value)
    {
        if (string.IsNullOrWhiteSpace(value))
        {
            throw new ArgumentException("A name can not be empty.");
        }
        return new Name(value);
    }
    public override string ToString() => _value;
}

This approach allowed me to design types safely, without immediately affecting the existing code. Once I was satisfied with the new types, I gradually integrated them into the main codebase.

4. First Class Collections: Assembling the Fellowship

I introduced a Fellowship class to manage the characters:

public sealed class Fellowship
{
    private readonly HashSet<Character> _members = [];

    public Fellowship AddMember(Character character)
    {
        if (!_members.Add(character))
        {
            throw new InvalidOperationException(
                "A character with the same name already exists in the fellowship.");
        }
        return this;
    }
    // ... other methods ...
}

Here is how the Service now looks like:

public class FellowshipOfTheRingService
{
    // Should be loaded from a Repository
    private readonly Fellowship _fellowship = new();

    public void AddMember(Character character) => _fellowship.AddMember(character);
    public void RemoveMember(string name) => _fellowship.Remove(name.ToName());

    public void MoveMembersToRegion(List<string> memberNames, string region)
        => _fellowship.MoveTo(
            region.ToRegion(),
            memberNames.Select(m => m.ToName()).ToArray()
        );

    public void PrintMembersInRegion(string region) => _fellowship.PrintMembersInRegion(region.ToRegion());
    public override string ToString() => _fellowship.ToString();
}

The Service is now lighter and its responsibility is to parse the inputs and pass them to the Domain.

5. No Getters/Setters/Properties: Teaching Objects to Do Things

I moved behavior into the domain objects, following the Tell, Don't Ask principle. This principle suggests that instead of asking an object for data and then acting on that data, we should tell the object what to do.

This encapsulates behavior with data and reduces coupling between objects.

public sealed class Character
{
    public void Move(Region destination, Logger logger)
    {
        if (CurrentLocation == Region.Mordor && destination != Region.Mordor)
        {
            throw new InvalidOperationException(
                $"Cannot move {Name} from Mordor to {destination}. Reason: There is no coming back from Mordor.");
        }
        CurrentLocation = destination;
        logger($"{Name} moved to {destination}.");
    }
    // ... other methods ...
}

Instead of getting the character's location, checking it, and then setting a new location, I tell the character to move. The character encapsulates the logic for movement, including any restrictions.

6. Embracing Monads: Wielding the Power of Either

To handle errors more gracefully, I introduced the Either monad using the LanguageExt library:

public Either<Error, Fellowship> AddMember(Character character)
    => !_members.Add(character)
        ? Error.New("A character with the same name already exists in the fellowship.")
        : this;

A monad is a design pattern that allows structuring programs generically. The Either monad represents a value that can be one of two types - typically a success value or an error value.

In this case, I used Either<Error, Fellowship> to represent either an Error (if adding the member fails) or the Fellowship (if it succeeds).

This approach allows to handle errors without throwing exceptions, making the code more predictable and easier to reason about. It forces to explicitly handle both the success and failure cases, leading to more robust code.

The Result: A Code Base Worthy of Rivendell

After this refactoring journey, I ended up with a codebase that was more expressive, robust, and maintainable (from my humble perspective). FellowshipOfTheRingService went from a jumbled mess to a well-organized system, each part playing its role like a member of the Fellowship.

public sealed class Fellowship
{
    private readonly Characters _members;

    public Fellowship() => _members = [];
    private Fellowship(Characters members) => _members = members;
    private Fellowship(Characters allMembers, Characters updated) => _members = allMembers + updated;

    public Either<Error, Fellowship> AddMember(Character character)
        => _members.Find(c => c == character)
            .Map(_ => Error.New("A character with the same name already exists in the fellowship."))
            .ToEither(defaultLeftValue: new Fellowship(_members.Add(character)))
            .Swap();

    public Either<Error, Fellowship> Remove(Name name)
        => _members.Find(character => character.HasName(name))
            .ToEither(defaultLeftValue: Error.New($"No character with the name '{name}' exists in the fellowship."))
            .Map(characterToRemove => new Fellowship(_members.Filter(c => c != characterToRemove)));

    public override string ToString()
        => _members.Fold("Fellowship of the Ring Members:\n",
            (current, member) => current + (member + "\n")
        );

    public Either<Error, Fellowship> MoveTo(Region destination, Logger logger, params Name[] names)
    {
        var membersToUpdate = _members.Filter(character => ContainsCharacter(names, character));
        var results = membersToUpdate.Map(character => character.Move(destination, logger));
        var errors = results.Lefts();

        return errors.Count != 0
            ? errors[0]
            : new Fellowship(_members.Filter(c => !membersToUpdate.Contains(c)), results.Rights());
    }

    private static bool ContainsCharacter(Name[] names, Character character)
        => names.Exists(character.HasName);

    public void PrintMembersInRegion(Region region, Logger logger)
    {
        var charactersInRegion = _members.Filter(m => m.IsIn(region));
        if (charactersInRegion.Count == 0)
        {
            logger($"No members in {region}");
            return;
        }

        logger($"Members in {region}:");

        charactersInRegion
            .ToList()
            .ForEach(character => logger(character.ToStringWithoutRegion()));
    }
}

public class FellowshipOfTheRingService(Logger logger)
{
    // holds the status of the fellowship
    // we update it on each successful call to behaviors
    private Fellowship _fellowship = new();

    public Either<Error, Unit> AddMember(Character character)
        => _fellowship
            .AddMember(character)
            .Do(f => _fellowship = f)
            .Map(_ => Unit.Default);

    public Either<Error, Unit> RemoveMember(string name)
        => name.ToName()
            .Bind(n => _fellowship.Remove(n))
            .Match(f => _fellowship = f, err => logger(err.Message));

    public Either<Error, Unit> MoveMembersToRegion(List<string> memberNames, string region)
        => _fellowship.MoveTo(
                region.ToRegion(),
                logger,
                memberNames.Map(m => m.ToName()).Rights().ToArray()
            )
            .Match(f => _fellowship = f, err => logger(err.Message));

    public void PrintMembersInRegion(string region) =>
        _fellowship.PrintMembersInRegion(region.ToRegion(), logger);

    public override string ToString() => _fellowship.ToString();
}

Conclusion: The Never-ending Journey

Refactoring, like the journey to destroy the One Ring, is never truly over. There's always room for improvement, always new challenges to face. But with each step, our code becomes a little cleaner, a little more robust, and a lot more enjoyable to work with.

Remember, in the words of Gandalf, "All we have to decide is what to do with the time that is given us."

So, fellow developers, let's use our time to craft code that's not just functional, but truly maintainable.

If you're interested in exploring this refactoringg further, check out the repository (look at the yot-calisthenics branch):

Here are all the explanations of the different steps I have followed to reach this result: https://github.com/ythirion/goat/tree/yot-calisthenics/object-calisthenics

We compared our results here:

If you want to see Pierre's implementation:

Have a goat day 🐐