Refactoring with baby steps

In a previous post we introduced the blessings Value Objects grant to our code. However, if we are not using them since the beginning we might ponder if it is worth messing up our whole codebase to introduce VOs. I can tell you it is totally worth it.

However, it is not that simple to introduce VOs (or any other change for that matter) into an existing codebase. In this post, we will see how to safely do it applying refactor techniques in small steps.

All the code changes presented here, and more, are published in this Github repository.

Precondition: test coverage

First things first, no matter the kind of change we want to introduce we need to be sure that we are not breaking our current code. The way of assuring it is through tests. Depending on the test coverage we have and its quality, on average we can face one of these three scenarios:

A) We have decent tests that cover the behavior of the code we want to change.
B) We have tests but they are coupled to the implementation of the code.
C) There are no tests at all.

If we are in scenario C, the first step to take is writing the tests. Ideally, the ones that put us in scenario A.

If we already have tests we can be in scenario A, which is a cozy environment for refactor and the code can be changed with little to no changes in tests. If we are in scenario B we will probably need a good deal of changes in the tests.

Introducing Value Objects can be considered a soft change in terms of impact on the component being modified. Value Objects apply when passing data around or operating with it. They don't substitute complex dependencies (e.g. accessing the database). We should expect a reduced amount of changes in the tests when introducing them, but that's highly dependant on the codebase we are dealing with.

The process

Refactoring in baby steps means we take small and safe steps, one at a time. Like babies do.

We will start in a green state (the tests are passing), and we will introduce one single change and run the tests. This change will be the smallest possible we can figure out that moves us in the direction of what we want to achieve. Sometimes they will mean a "step back" in terms of code quality, like introducing duplication, optional parameters, weird method naming, etc. But that's ok because those are temporal intermediate states until we reach our refactor purpose.

After any change, if the tests are still green we happily proceed to introduce the next change. If the tests become red, we will know exactly which change caused it. Either we undo it and start over from the last green state, which will be only one change away, or we identify the reason for the error and correct it.

Most of the time the smallest possible change is not bigger than a single line. Identifying the reason for a red in the tests becomes quite easy because the amount of code directly affected is small. When we modify larger chunks of code it is more likely we would not know where we introduced an error and we would invest more time, on average, to figure it out.

Should I apply baby steps always?

No. But...

During the process described here, some of the steps may look ridiculous and we could think we could take a group of them and apply them together straight away. That's fine as long as we do it consciously. That we know we are tackling a number of smaller steps at once, and we have the mental discipline to roll back the changes and start over, one small step at a time if it is needed. We cannot get to this position unless we practice enough applying baby steps to the point we internalize it.

We will notice we take a lot of steps to reach our refactor goal and we don't spend a lot of time fixing possible errors. Because the time spent fixing wrong decisions is minimal (because the decisions are minimal as well) we will be advancing at a continuous pace. The process is overall faster and safer than taking a big chunk of changes at once and having to think about what we did wrong when it does not work.

With practice comes experience, and with experience comes the confidence to take bigger steps. But we should always have the baby steps tool ready if things get messy.

The code we will refactor

Now we will apply refactoring with baby steps in a simple code example but juicy enough to showcase the process, set in the context of the aforementioned Value Objects post: a Jira-like software managing Story Points as primitive int values.

The method we will refactor is:

class SprintReport {
    public function __construct(
        private int $completedStoryPoints,
    ){}

    public function addCompletedStoryPoints(int $amount): void
    {
        if ($amount < 0) {
            throw new InvalidArgumentException('SP cannot be less than 0');
        }

        $this->completedStoryPoints += $amount;
    }
}

Diligent as we are, it is covered by the following (truncated) tests :

final class SprintReportTest extends TestCase
{
    /** @test */
    public function completedPointsDoNotChangeWhenAddingZeroStoryPoints(): void { ... }

    /** @test */
    public function failsWhenAddingLessThanZeroStoryPoints(): void { ... }

    /** @test */
    public function registersStoryPointsWhenAddingAValidAmountToAnEmptyReport(): void { ... }

    /** @test */
    public function addsUpStoryPointsWhenAddingToANonEmptyReport(): void { ... }
}

For reference, this is how a test actually looks like:

final class SprintReportTest extends TestCase
{
    private SprintReport $sprintReport;

    protected function setUp(): void
    {
        parent::setUp();
        $this->sprintReport = new SprintReport(0);
    }

    /** @test */
    public function addsUpStoryPointsWhenAddingToANonEmptyReport(): void
    {
        $this->sprintReport->addCompletedStoryPoints(3);
        $this->sprintReport->addCompletedStoryPoints(5);

        $expected = new SprintReport(8);
        self::assertEquals($expected, $this->sprintReport);
    }
}

And our StoryPoint Value Object:

class StoryPoint
{
    public function __construct(private int $value) { }

    public function value(): int
    {
        return $this->value;
    }
}

The first refactor

Now that the ground is settled, we start to refactor. We will refer to the current implementation as the "old method" and the one we are introducing as the "new method".

Step 1: Adding an empty method

A new empty method that accepts our StoryPoint VO as parameter. Nobody is using it yet so it should not break anything. We run the tests and indeed they are still green.

Step 2: Instantiate a StoryPoint using the int value

In the old method, we instantiate a StoryPoint from the int parameter. Tests are green, life is simple.

These two first steps may seem stupid. Well, we don't think (at least I hope so) babies are stupid because they start walking with small steps that are extremely conservative compared to their usual crawling. They are afraid of failing and so we are. They have pretty much less to lose than us being cautious, and still they are. There is something to learn here.

Indeed, with time and experience, we will happily be applying these two steps at once without hesitation.

Step 3: Duplicate the guard clause in the new method

This one is more interesting. We duplicate the clause guard checking the value of the story point is valid in our new method. Because the value is encapsulated in the VO, we check it using the getter we have for it.This is not a good approach from a VO point of view, the VO should be self-validated and the fact we have an instance means it is valid. In other words, the validation should happen inside the VO. But our focus right now is to get rid of the old method using primitives and replace it with usages of our VO. After that, we will be in a better position to apply a second refactor and make our VO self validated.

The tests are still green.

Step 4: Call the new method after the guard clause

Now that we actually call the code we duplicated in the previous step and the tests are green, we know that our duplicated guard works as expected when the input is valid. However, we don't know if we did a mistake when checking invalid input since the guard in the old method will fail and prevent our new method from being executed. But we are in an optimal scenario to verify our new guard validates correctly.

Step 5: Delete the old guard clause

Actually, the recommended approach here would be to comment out the old guard, run the test, check they are green, and actually remove the old guard clause. Commenting code is safer than cutting text or rely on the undo operation of our editor. When we comment we always "move forward" and the code is "persisted" instead of going backward with undo or put our hopes in the hands of the volatility of the clipboard.

When we totally delete the old guard clause and verify the tests are green, we can confirm our new guard clause correctly validates both valid and invalid input.

Step 6: Duplicate and comment out the increase of story points

Replacing this part of the old method is tricky: we cannot keep the old one and the new one at the same time because it is an operation that modifies the state of our SprintReport and keeping both would do the modification twice. We duplicate it adapted to the usage of StoryPoint but commented out. Doing so we can easily switch between both implementations to verify tests are green and go back to the old one in case they are not.

With the commented new code tests are still green.

Step 7: Uncomment the new increase, delete the old one

Doing the switch between implementations commenting and uncommenting each of them allows us to run the tests for each with the ability to switch back if we broke something in our new code. Once we verify we did not, we can delete the old implementation for good.

Step 8: Update the tests to use the new method

At this point, we have a new method using the VO as parameter and an old method that only instantiates the VO from the int parameter and calls the new method. We can now replace all of the usages of the old method with the new one. The spectrum of this change will affect the tests and the whole application.

Depending on how many times the old method is used this change could imply a lot of updates in different parts of the application and may not be a good mass update to tackle at this point. Maybe it is better to iteratively update its remaining usages when going through the code using it while we work on other tasks. We can make this explicit for future devs renaming the method to something that evidences it is recommended to use the new method or flagging it as deprecated.

In any case, the following steps are not especially sexy: automated renames using the IDE and "search and replace" usages of the old method. We will skip them here, but you can check them in the repository.

The second refactor

Although we have a method expecting a StoryPoint that substitutes the old one dealing with primitives, we still have room for improvement in other areas:

• The SprintReport still uses int for tracking the Story Points.
• The validation of a correct amount of Story Points happens outside the VO.
• The addition of Story Points in a report uses the internal value of the VO, instead of doing the addition using a add() method exposed by the StoryPoint.

Covering all this stuff would take too much for this already too long post, so we will focus on the first one showcasing another technique to apply baby steps when refactoring: adding optional parameters with a default value.

Step 1: Add a new optional StoryPoint parameter to the constructor

The new parameter defaults to null, hence any previous usage of the constructor won't fail and tests are green. Doing so we allow to introduce usages of the new constructor parameter and adapt the class to use it while not breaking any existing code.

Step 2: Conditionally set the initial story points from the VO parameter when provided

Now our constructor sets the initial Story Points from the optional parameter when it is provided. Nobody does so yet and the tests are green. At this point, we are in the position of iteratively provide the optional parameter while keeping our SprintReport functional.

Step 3: Update the tests to provide the optional parameter

Since this is an educational post our class is quite simple and only used in the tests, so we just need to update them. However, in reality, there would be more usages of SprintReport, and adapting all of them might be a huge effort.

An alternative to going over all SprintReport constructor usages is to add a new static factory method to it, allowing to incrementally update our codebase to use the VO typed parameter while keeping compatibility with the int one:

class SprintReport
{
    public static function withCompleteStoryPoints(StoryPoint $completed): self
    {
        return new self($completed->value(), $completed);
    }
}

Since here we only have the tests using it, we update them right away.

Step 4: Duplicate and comment the assignment of completed Story Points

With this step we will allow, by switching between the conditional assignment and this new one, to verify all the usages of SprintReport constructor are updated. When commenting the conditional block and using the new one, any test covering a usage that is not providing the VO parameter will fail.

Step 5: Delete the conditional block from constructor

Once we are sure all the constructor usages provide the VO parameter, we can delete the conditional assignment.

At this point we can perform automated refactors with our IDE, changing the constructor signature to accept only the VO parameter and completing our change to remove the usage of Story Points as integers.

More refactors

As mentioned before, there are several other changes we could do in SprintReport and in StoryPoint to improve our implementation and usage of Value Objects for this case.

That would be too much to describe here, but you can check in the repository the next steps to make the most out of the encapsulation Value Object pattern provides. In small and safe steps :D

Final words

A TL;DR version of this post could be:

• Always have tests in place.
• Apply the smaller change possible, run the tests. They will be probably green because the change was minimal.
• If tests are not green, fixing or going back to the previous step is trivial because the change was minimal.
• We save time refactoring in baby steps.
• Maybe we don't notice we save time just because we don't spend time figuring out what we did wrong when introducing a non-trivial change.
• With practice and experience, we will be able to take bigger steps. But we should do it consciously and be able to fall back to baby steps if things get messy.

As with any stuff, it will take some time to get used to this type of approach to refactoring. As with the good stuff, once you are into it it is difficult to turn back.

For a more profound and consistent read about refactoring and object-oriented programming in general, I can recommend you the book 99 bottles of OOP by Sandy Metz, Katrina Owen, and TJ Stankus. I recommend it even if you have no interest in refactoring with baby steps. It may be one of the best books about writing OOP out there.

And that's all for today. As always, feedback is much appreciated. See you!

PS: I talk about Story Points here because it is a handy example, but I discourage using them. And so do the people who invented them. You have been warned.

PS2: If anyone knows any decent software to generate visually appealing diff images, please let me know. Going screenshot as I did here is hell.