Don’t Refactor Like Uncle Bob. Please

Correction: Throughout this article, I attribute Chapter 2 of Clean Code to Robert Martin, however I was recently informed that this particular chapter was actually authored by Tim Ottinger. That said, Martin is listed as the sole author of this book, so it’s a given that he stands by the advice from this chapter.

“Clean Code” has garnered a bit of notoriety despite coining an endearing term we use in coding conversations. This book from 2008 is a compilation of principles and studies that “Uncle Bob” (a.k.a Robert Martin) had developed in his decades of programming.

Many have incorporated his practices into their work, some swearing by it, and others feeling that they’re to be taken more as loose guidelines rather than rules. Regardless of what you think, though, Uncle Bob doesn’t see them as guidelines. He ascribes to them wholeheartedly and very rarely allows exceptions.

So you’d think that his examples of refactoring in this book would at least be better than the average code you see in your day-to-day work. Or at least consistent with his other advices.

You’d think.

Keep in mind that Martin didn’t make up these examples out of the blue. These are ACTUAL changes he’s made in his work, and he’s using them as teachable moments.

I’ll be discussing the very first example of refactoring in his book from “Chapter 2: Meaningful Names”. Here’s the before:

private void printGuessStatistics(char candidate, int count) {
    String number;
    String verb;
    String pluralModifier;
    if (count == 0) {
        number = "no";
        verb = "are";
        pluralModifier = "s";
    } else if (count == 1) {
        number = "1";
        verb = "is";
        pluralModifier = "";
    } else {
        number = Integer.toString(count);
        verb = "are";
        pluralModifier = "s";
    }
    String guessMessage = String.format(
        "There %s %s %s%s", verb, number, candidate, pluralModifier
    );
    print(guessMessage);
}

and here’s after:

public class GuessStatisticsMessage {
    private String number;
    private String verb;
    private String pluralModifier;

    public String make(char candidate, int count) {
        createPluralDependentMessageParts(count);
        return String.format(
            "There %s %s %s%s", 
            verb, number, candidate, pluralModifier );
    }

    private void createPluralDependentMessageParts(int count) {
        if (count == 0) {
            thereAreNoLetters();
        } else if (count == 1) {
            thereIsOneLetter();
        } else {
           thereAreManyLetters(count);
        }
    }

    private void thereAreManyLetters(int count) {
        number = Integer.toString(count);
        verb = "are";
        pluralModifier = "s";
    }

    private void thereIsOneLetter() {
        number = "1";
        verb = "is";
        pluralModifier = "";
    }

    private void thereAreNoLetters() {
        number = "no";
        verb = "are";
        pluralModifier = "s";
    }
}

Oh boy.

Before I get into what’s wrong with this, let me say something first.

There’s a whole other chapter about function separation, but it somehow leaked into this one. Martin does this a lot in his examples. His strict adherence to using real code changes from his work confounds the lessons he teaches in each chapter. He could have had these functions laid out to begin with and simply changed names.

And in many of his examples, he’ll show “before” code that’s bad for significant reasons than just the ones he teaches in that chapter, but he’ll fix those too, which makes it harder for a reader to judge just how effective that chapter’s advice really is on its own.

I tell you all this because I’ll evaluate this code holistically, just like he refactors holistically in his examples. I think that’s fair.

The first thing you’ll notice is that Martin has taken a single, mostly PURE function (shout out to all the functional bros), and made a class out of it. And not a static utility class or anything like that. An INSTANCE, with its attributes being mutated to serve the result.

Why does he do this? Well, it’s because Martin’s relentless insistence on splitting out functions and having zero arguments means that class attributes are the only way to assign variables inside a function to be used outside that function. Java doesn’t have pass-by-references, except with classes, but Martin doesn’t even want to pass a class as an argument and mutate that, so the end result is littered with side effects (i.e functions modifying outside their scopes).

He also seems to pick and choose when he uses instance variables and when he passes arguments. Like why is count an argument and not also assigned to an instance variable? He never explains his reasoning for this. I can only guess it’s because count is passed in from outside the class, but why does that matter? Having it as an instance variable means one less argument to pass around, which is what Martin seems to be going for.

Am I supposed to interpret make as a constructor? That can’t be right, because it returns a String, not a GuessStatisticsMessage type. If he’d made a case for these instance variables needing to exist for something other than this specific return string, I might’ve been on board with this. But he never does.

The second thing you’ll notice is that even though this chapter is about naming, the names are, ironically, not that good.
What does make mean?
What do number, verb, and pluralModifier mean in the context of the problem?
Why do functions that modify state start with the word “there”?
How is a reader supposed to know what GuessStatisticsMessage means without the calling context? And why is the name a weird verb-noun hybrid?
I could go on.

If you’re like me, you had to read all the code anyway to understand what make does. It generates a grammatically correct sentence describing the count of a given character (i.e “There are 4 Cs”, “There are no Ds”, “There is 1 B”, etc). That’s all. I could’ve told you that more easily just by looking at the original code.

The last thing you’ll notice is that Martin hasn’t actually made the code easier to understand. Because even if his naming was on point, there is now more code to parse.

The spaces between functions make the logic no longer fit on a screen. This adds an overhead of visual memory to piece the functionality together.

The functions themselves add an overhead of finding where they’re called, along with having to track the arguments passed to them. Or worse, track the mutations of instance variables.

Just reading the body of createPluralDependentMessageParts (what a mouthful) doesn’t actually tell you what’s going on under the hood. That’s because Martin considers the if blocks to be in a lower level of abstraction than the enclosing function, so he believes they should be encapsulated (i.e hidden) in their own functions.

Now, imagine you’re trying to understand this code for the first time. You reach the if blocks and see that they delegate to other methods. Are you gonna stop there? Probably not.

Anyone who’s read the code up to this point would love to know what’s inside thereAreNoLetters(). In fact, you cannot understand the make method without understanding how verb, number, and pluralModifier are mutated. You might be able to guess by looking at the return string command and the order in which the fields are used. But there’s no way for you to know that “no” replaces “0” unless you go deeper. This is an essential detail to understanding make, and it’s encapsulated 2 levels deep.

This unintuitive separation results from Martin treating levels of abstraction so granularly, that the most microscopic of dips rings alarm bells. But that’s the thing; it’s just him. Most programmers would rather deviate a little in abstraction, than to fragment a simple function into a stateful, side-effect-ridden tree of methods.

The original method was actually better. By far. But Martin’s right in that it could be improved, he just went about it the wrong way. Here’s my “perfect” version:

private String generateGuessSentence(char candidate, int count) {
    String verbIs = count == 1 ? "is" : "are";
    String countStr = count == 0 ? "no" : Integer.toString(count);
    String sIfPlural = count == 1 ? "" : "s";
    return String.format(
        "There %s %s %s%s",
        verbIs, countStr, candidate, sIfPlural
    );
}

You may have noticed the subtle redundancies in the original code, namely that each String value is the same in 2 of the 3 cases. One of Martin’s rules is that you should reduce duplication wherever possible, and he breaks it here for no reason.

You may have also noticed that these if statements don’t do anything except assign values to variables. Martin uses ternary operators in other examples, so why not here? I can only assume it’s because he found the 3 count cases to be clearer than assigning each value with individual conditions like I have.

Strange, isn’t it? Martin extracted the logic into functions before even trying to optimize it first. I can’t read his mind, but in the book, he almost never mentions alternative approaches to refactoring. He simply shows his improved version, and explains why it’s the best with surface-level explanations. I feel like this should’ve been a major lesson.

But if Martin really wants to keep the 3 cases, the naming, and the other functions, I can play along with that. I still think his refactoring could have been cleaner, even by his own standards. Here’s another version I made:

public class GuessStatisticsMessage {
    private char candidate;
    private int count;

    public GuessStatisticsMessage(char candidate, int count) {
        this.candidate = candidate;
        this.count = count;
    }

    public String make() {
        if (count == 0) {
            return thereAreNoLetters();
        } else if (count == 1) {
            return thereIsOneLetter();
        } else {
            return thereAreManyLetters();
        }
    }

    private String thereAreManyLetters() {
        return String.format(
            "There are %s %ss", count, candidate
        );
    }

    private String thereIsOneLetter() {
        return String.format(
            "There is 1 %s", candidate
        );
    }

    private String thereAreNoLetters() {
        return String.format(
            "There are no %ss", candidate
        );
    }
}

Would you look at that. A proper constructor, no arguments, and all the string operations are in the lowest-level methods. Now that’s clean code👌! Just ignore that there’s no reason for this to be a class.

So in conclusion, if you’re gonna read this book, ignore the refactorings and come up with your own. And take his principles with a grain of salt.

P.S
I can’t believe this is my first post!

My inspiration for writing this article came from noticing how often “Clean Code” was being uncritically recommended to beginners. Fortunately, I’ve never worked with anyone who coded like this. But the thought of aspiring devs learning from this book scares me for the future of the field.

Credibility of opinion about software development practices is to be viewed with skepticism. Software as a field is in its infancy and is constantly changing. By the time you become an expert, the landscape has already shifted.

I was also inspired by Martin’s recent interview with a Youtuber named “ThePrimeTime”. They talked about a lot of stuff, and it was mostly just a casual conversation, though Primagen tried to challenge some of Martin’s principles at some points throughout.

So I was expecting that Martin, 16 years after his book, would relax some of his more…debatable principles. But he didn’t. He doubled down, and that was the last straw for me. Look, I don’t have anything personal against him. I just think there aren’t enough people calling out his weaker advice.

Anyway, thanks for reading and have a nice day.