When to Refactor

How to maintain the balance between adding new features and refactoring existing code? Here are some rules of thumb for choosing when to refactor and how much.

Refactoring is the process of improving the code's design without affecting its functionality. Is it possible to over-refactor? I don't think that code can ever be "too clean", and following the four elements of Simple Design should not result in over-engineering. But certainly there is code that needs more cleaning up than other cases, and we rarely have enough time to do all we want. That's why prioritization is needed.

When I refactor, it's usually in one of the following situations.

After getting a test to pass

At unit scale TDD (as opposed to system scale TDD), writing a test and making it pass takes only a few minutes (or you're working in too big steps). After getting a test to pass, it's good to take a moment to look at the code we just wrote and clean it up. Basically it comes down to removing duplication and improving names, i.e. Simple Design. This takes just a minute or two.

This is also a good time to fix any obvious design smells while they are still small. For example Primitive Obsession gets the harder to fix the more widespread it is. This usually takes just a few minutes and at most an hour. Very faint design smells I would leave lying around until they ripen enough for me to know how to fix them - but not too long, so that they begin to rot.

When adding a feature is hard

If the system's design does not make it easy to add a feature I'm currently working on, I would first refactor the system to make adding that feature easier. If this is the second or third instance of a similar feature [1], I would refactor the code to follow the Open-Closed Principle, so that in the future adding similar features will be trivial. This might take from half an hour up to a couple of hours.

When the difficulty of adding a feature hits you right away like a ton of bricks, then it's obvious to do the refactoring first. But what if a difficulty sneaks up on you during implementing the feature? Trying to refactor and implement features at the same time is a road to pain and suffering. Instead, retreat to the last time that all tests passed (either revert/stash your changes or disable the new feature's one failing test), after which you can better focus on keeping the tests green while refactoring.

When our understanding of what would be the correct design improves

When we start developing a program, we have only partial understanding of the problem being solved, but we'll do our best to make the code reflect our current understanding of the problem. As the program grows over months and years, we will learn more and inevitably there will be parts of the code that we would have designed differently, if we only had then known what we know today. This is the original definition of the Technical Debt metaphor and the ability to pay back the debt depends on how clean the code is.

For big refactorings, it is unpractical to block adding new features while the design is being changed. So working towards a new design should be done incrementally at the same time as developing new features. Whenever a developer needs to change code that does not yet conform to the target design, they should refactor that part of the codebase there and then, before implementing the feature at hand. This way it might take many weeks or months for the whole codebase to be refactored, but it is done incrementally in small steps, a class or a method at a time (which should not take more than a couple of hours), so that the software keeps working at all times.

When trying to understand what some piece of code does

If you need to understand some code, even if you're not going to change it, refactoring the code is one means for understanding it better. Extract methods and variables, give them better names and move things around until the code says clearly what it does. You may combine this with writing unit tests, which likewise helps to understand the code.

If the code has good test coverage, you might as well commit the changes you just did, in hopes of the next reader understanding the code faster [2]. But even if the code has no tests, you can do some refactoring to understand it and then throw away your changes - your understanding will remain. If you know that you're going to throw away your changes, you can even do the throwaway refactoring faster with less care. And for complex refactorings, when you're not sure about what sequence of steps would bring you safely to your goal, prodding around the code can help you to get a feel for the correct refactoring sequence.

TL;DR

Refactoring does not have to be, nor should be, its own development phase which takes weeks or months. Instead it can be done incrementally in small steps, interleaved with feature development.

---

Notes

[1]: If the shape of the code is developing into a direction that you've seen happen many times in the past, it's easy to know how to refactor it already when the second duplicate instance raises its head. But if you're uncertain of what the code should be like, it may be worthwhile to leave the second duplicate be and wait for the third duplicate before creating a generic solution, so that you can clearly see which parts are duplicated and which vary.

[2]: Sometimes I wonder whether a refactoring made the code better, or I just understand it better because of spending time refactoring it.

---

This article was first published in the Solita developer blog. There you can find also other articles like this.

Phase Change Pattern for Mutating Immutable Objects

Here is a design pattern I've found useful for "mutating" immutable objects in non-functional languages, such as Java, which don't have good support for it (unlike for example Scala's copy methods). I shall call it the Phase Change Pattern, because of how it freezes and melts objects to make them immutable and back again mutable, the same way as water can be frozen and melted.

This pattern consist of the following parts:

• An immutable class with some properties
• A mutable class with the same properties
• The mutable class has a freeze() method for converting it to the immutable class
• The immutable class has a melt() method for converting it to the mutable class
• Both of the classes have package-private copy constructors that take the other class' instance as parameter, copying all fields (making immutable/mutable copies of the field values when necessary)

The freeze method plays a similar role as the build method in the Builder pattern, but it's named after Ruby's freeze method. The name melt was chosen as a metaphor based on the thermodynamic phase changes. I think it has better connotations than the other alternatives I considered: "build - destroy", "save - edit", "persist - dispersist" (or can "transient" be made a verb?)

Code Example

The following code is taken from the Jumi Test Runner project where I originally invented this pattern.

Usage

The classes can be used like this to create nice immutable classes that may be freely passed around:

SuiteConfiguration config = new SuiteConfigurationBuilder()
        .addToClasspath(Paths.get("something.jar"))
        .addJvmOptions("-ea")
        .freeze();

But the pattern also makes it possible, in a method that takes the immutable object as parameter, to augment it with new values:

config = config.melt()
        .addJvmOptions("-javaagent:extra-agent.jar")
        .freeze();

This is useful in situations where the code that creates the original immutable object does not know all the arguments, but some of the arguments are known only much later by some other code.

Immutable Class

Here is the immutable class. Its default constructor sets all fields to their default values. The copy constructor will need to make immutable copies of all mutable properties (e.g. java.util.List). The copy constructor takes the builder as parameter, making it easier to match the field names than if each property had its own constuctor parameter. The cyclic dependency is a small price to pay for this convenience. There are getters for all properties. Also this class can be made a value object by overriding equals, hashCode and toString.

@Immutable
public class SuiteConfiguration {

    public static final SuiteConfiguration DEFAULTS = new SuiteConfiguration();

    private final List<URI> classpath;
    private final List<String> jvmOptions;
    private final URI workingDirectory;
    private final String includedTestsPattern;
    private final String excludedTestsPattern;

    public SuiteConfiguration() {
        classpath = Collections.emptyList();
        jvmOptions = Collections.emptyList();
        workingDirectory = Paths.get(".").normalize().toUri();
        includedTestsPattern = "glob:**Test.class";
        excludedTestsPattern = "glob:**$*.class";
    }

    SuiteConfiguration(SuiteConfigurationBuilder src) {
        classpath = Immutables.list(src.getClasspath());
        jvmOptions = Immutables.list(src.getJvmOptions());
        workingDirectory = src.getWorkingDirectory();
        includedTestsPattern = src.getIncludedTestsPattern();
        excludedTestsPattern = src.getExcludedTestsPattern();
    }

    public SuiteConfigurationBuilder melt() {
        return new SuiteConfigurationBuilder(this);
    }

    @Override
    public boolean equals(Object that) {
        return EqualsBuilder.reflectionEquals(this, that);
    }

    @Override
    public int hashCode() {
        return HashCodeBuilder.reflectionHashCode(this);
    }

    @Override
    public String toString() {
        return ToStringBuilder.reflectionToString(this, ToStringStyle.SHORT_PREFIX_STYLE);
    }


    // getters

    public List<URI> getClasspath() {
        return classpath;
    }

    public List<String> getJvmOptions() {
        return jvmOptions;
    }

    public URI getWorkingDirectory() {
        return workingDirectory;
    }

    public String getIncludedTestsPattern() {
        return includedTestsPattern;
    }

    public String getExcludedTestsPattern() {
        return excludedTestsPattern;
    }
}

Mutable Class

Here is the mutable class. Its constructors and freeze method are a dual to the immutable class' constructors and melt method. The defaults are written only once, in the immutable class. This class has both getters and setters for all the properties. All the mutator methods return this to enable method chaining.

@NotThreadSafe
public class SuiteConfigurationBuilder {

    private final List<URI> classpath;
    private final List<String> jvmOptions;
    private URI workingDirectory;
    private String includedTestsPattern;
    private String excludedTestsPattern;

    public SuiteConfigurationBuilder() {
        this(SuiteConfiguration.DEFAULTS);
    }

    SuiteConfigurationBuilder(SuiteConfiguration src) {
        classpath = new ArrayList<>(src.getClasspath());
        jvmOptions = new ArrayList<>(src.getJvmOptions());
        workingDirectory = src.getWorkingDirectory();
        includedTestsPattern = src.getIncludedTestsPattern();
        excludedTestsPattern = src.getExcludedTestsPattern();
    }

    public SuiteConfiguration freeze() {
        return new SuiteConfiguration(this);
    }


    // getters and setters

    public List<URI> getClasspath() {
        return classpath;
    }

    public SuiteConfigurationBuilder setClasspath(URI... files) {
        classpath.clear();
        for (URI file : files) {
            addToClasspath(file);
        }
        return this;
    }

    public SuiteConfigurationBuilder addToClasspath(URI file) {
        classpath.add(file);
        return this;
    }

    ...
}

Most of the mutators have been omitted for brevity. The full source code of these classes is in Github.