It may not be possible for us to ever reach empirical definitions of "good code" or "clean code", which means that any one person's opinions about another person's opinions about "clean code" are necessarily highly subjective. I cannot review Robert C. Martin's 2008 book Clean Code from your perspective, only mine.

That said, the major problem I have with Clean Code is that a lot of the example code in the book is just dreadful.

*

In chapter 3, "Functions", Martin gives a variety of advice for writing functions well. Probably the strongest single piece of advice in this chapter is that functions should not mix levels of abstraction; they should not perform both high-level and low-level tasks, because this is confusing and muddles the function's responsibility. There's other valid stuff in this chapter: Martin says that function names should be descriptive, and consistent, and should be verb phrases, and should be chosen carefully. He says that functions should do exactly one thing, and do it well, which I agree with... provided that we aren't too dogmatic about how we define "one thing", and we understand that in plenty of cases this can be highly impractical. He says that functions should not have side effects (and he provides a really great example), and that output arguments are to be avoided in favour of return values. He says that functions should generally either be commands, which do something, or queries, which answer something, but not both. This is all reasonable entry-level advice.

But mixed into the chapter there are more questionable assertions. Martin says that Boolean flag arguments are bad practice, which I agree with, because an unadorned true or false in source code is opaque and unclear versus an explicit IS_SUITE or IS_NOT_SUITE... but Martin's reasoning is rather that a Boolean argument means that a function does more than one thing, which it shouldn't.

Martin says that it should be possible to read a single source file from top to bottom as narrative, with the level of abstraction in each function descending as we read on, each function calling out to others further down. This is far from universally relevant. Many source files, I would even say most source files, cannot be neatly hierarchised in this way. And even for the ones which can, an IDE lets us trivially jump from function call to function implementation and back, the same way that we browse websites.

He says code duplication "may be the root of all evil in software" and fiercely advocates DRY. At the time, this was quite standard advice. In more recent times, however, we generally understand that a little duplication isn't the worst thing in the world; it can be clearer, and it can be cheaper than the wrong abstraction.

And then it gets weird. Martin says that functions should not be large enough to hold nested control structures (conditionals and loops); equivalently, they should not be indented to more than two levels. He says blocks should be one line long, consisting probably of a single function call. He says that an ideal function has zero arguments (but still no side effects?), and that a function with just three arguments is confusing and difficult to test. Most bizarrely, Martin asserts that an ideal function is two to four lines of code long. This piece of advice is actually placed at the start of the chapter. It's the first and most important rule:

The first rule of functions is that they should be small. The second rule of functions is that they should be smaller than that. This is not an assertion that I can justify. I can’t provide any references to research that shows that very small functions are better. What I can tell you is that for nearly four decades I have written functions of all different sizes. I’ve written several nasty 3,000-line abominations. I’ve written scads of functions in the 100 to 300 line range. And I’ve written functions that were 20 to 30 lines long. What this experience has taught me, through long trial and error, is that functions should be very small.

[...]

When Kent [Beck] showed me the code, I was struck by how small all the functions were. I was used to functions in Swing programs that took up miles of vertical space. Every function in this program was just two, or three, or four lines long. Each was transparently obvious. Each told a story. And each led you to the next in a compelling order. That’s how short your functions should be!

All of this sounds like hyperbole. A case for short functions instead of long ones can certainly be made, but we assume that Martin doesn't literally mean that every function in our entire application must be four lines long or less.

But the book is being absolutely serious about this. All of this advice culminates in the following source code listing at the end of chapter 3. This example code is Martin's preferred refactoring of a pair of Java methods originating in an open-source testing tool, FitNesse.

package fitnesse.html;

import fitnesse.responders.run.SuiteResponder;
import fitnesse.wiki.*;

public class SetupTeardownIncluder {
  private PageData pageData;
  private boolean isSuite;
  private WikiPage testPage;
  private StringBuffer newPageContent;
  private PageCrawler pageCrawler;


  public static String render(PageData pageData) throws Exception {
    return render(pageData, false);
  }

  public static String render(PageData pageData, boolean isSuite)
    throws Exception {
    return new SetupTeardownIncluder(pageData).render(isSuite);
  }

  private SetupTeardownIncluder(PageData pageData) {
    this.pageData = pageData;
    testPage = pageData.getWikiPage();
    pageCrawler = testPage.getPageCrawler();
    newPageContent = new StringBuffer();
  }

  private String render(boolean isSuite) throws Exception {
     this.isSuite = isSuite;
    if (isTestPage())
      includeSetupAndTeardownPages();
    return pageData.getHtml();
  }

  private boolean isTestPage() throws Exception {
    return pageData.hasAttribute("Test");
  }

  private void includeSetupAndTeardownPages() throws Exception {
    includeSetupPages();
    includePageContent();
    includeTeardownPages();
    updatePageContent();
  }


  private void includeSetupPages() throws Exception {
    if (isSuite)
      includeSuiteSetupPage();
    includeSetupPage();
  }

  private void includeSuiteSetupPage() throws Exception {
    include(SuiteResponder.SUITE_SETUP_NAME, "-setup");
  }

  private void includeSetupPage() throws Exception {
    include("SetUp", "-setup");
  }

  private void includePageContent() throws Exception {
    newPageContent.append(pageData.getContent());
  }

  private void includeTeardownPages() throws Exception {
    includeTeardownPage();
    if (isSuite)
      includeSuiteTeardownPage();
  }

  private void includeTeardownPage() throws Exception {
    include("TearDown", "-teardown");
  }

  private void includeSuiteTeardownPage() throws Exception {
    include(SuiteResponder.SUITE_TEARDOWN_NAME, "-teardown");
  }

  private void updatePageContent() throws Exception {
    pageData.setContent(newPageContent.toString());
  }

  private void include(String pageName, String arg) throws Exception {
    WikiPage inheritedPage = findInheritedPage(pageName);
    if (inheritedPage != null) {
      String pagePathName = getPathNameForPage(inheritedPage);
      buildIncludeDirective(pagePathName, arg);
    }
  }

  private WikiPage findInheritedPage(String pageName) throws Exception {
    return PageCrawlerImpl.getInheritedPage(pageName, testPage);
  }

  private String getPathNameForPage(WikiPage page) throws Exception {
    WikiPagePath pagePath = pageCrawler.getFullPath(page);
    return PathParser.render(pagePath);
  }

  private void buildIncludeDirective(String pagePathName, String arg) {
    newPageContent
      .append("\n!include ")
      .append(arg)
      .append(" .")
      .append(pagePathName)
      .append("\n");
  }
}

I'll say again: this is Martin's own code, written to his personal standards. This is the ideal, presented to us as a learning example.

I will confess at this stage that my Java skills are dated and rusty, almost as dated and rusty as this book, which is from 2008. But surely, even in 2008, this code was illegible trash?

Let's ignore the wildcard import.

First, the class name, SetupTeardownIncluder, is dreadful. It is, at least, a noun phrase, as all class names should be. But it's a nouned verb phrase, the strangled kind of class name you invariably get when you're working in strictly object-oriented code, where everything has to be a class, but sometimes the thing you really need is just one simple gosh-danged function.

Inside the class, we have:

• two public, static methods, as before, plus
• one new private constructor and
• fifteen new private methods.

Of the fifteen private methods, fully thirteen of them either have side effects (such as buildIncludeDirective, which has side effects on newPageContent) or call out to other methods which have side effects (such as include, which calls buildIncludeDirective). Only isTestPage and findInheritedPage look to be side-effect-free. They still make use of variables which aren't passed into them (pageData and testPage respectively) but they appear to do so in side-effect-free ways.

At this point you might reason that maybe Martin's definition of "side effect" doesn't include member variables of the object whose method we just called. If we take this definition, then the five member variables, pageData, isSuite, testPage, newPageContent and pageCrawler, are implicitly passed to every private method call, and they are considered fair game; any private method is free to do anything it likes to any of these variables.

But Martin's own definition contradicts this. This is from earlier in this exact chapter, with emphasis added:

Side effects are lies. Your function promises to do one thing, but it also does other hidden things. Sometimes it will make unexpected changes to the variables of its own class. Sometimes it will make them to the parameters passed into the function or to system globals. In either case they are devious and damaging mistruths that often result in strange temporal couplings and order dependencies.

I like this definition. I agree with this definition. It's a useful definition, because it enables us to reason about what a function does, with some degree of confidence, by referring only to its inputs and output. I agree that it's bad for a function to make unexpected changes to the variables of its own class.

So why does Martin's own code, "clean" code, do nothing but this? Rather than have a method pass arguments to another method, Martin makes a distressing habit of having the first method set a member variable which the second method, or some other method, then reads back. This makes it incredibly hard to figure out what any of this code does, because all of these incredibly tiny methods do almost nothing and work exclusively through side effects.

Let's just look at one private method.

private String render(boolean isSuite) throws Exception {
   this.isSuite = isSuite;
  if (isTestPage())
    includeSetupAndTeardownPages();
  return pageData.getHtml();
}

So... imagine that someone enters a kitchen, because they want to show you how to make a cup of coffee. As you watch carefully, they flick a switch on the wall. The switch looks like a light switch, but none of the lights in the kitchen turn on or off. Next, they open a cabinet and take down a mug, set it on the worktop, and then tap it twice with a teaspoon. They wait for thirty seconds, and finally they reach behind the refrigerator, where you can't see, and pull out a different mug, this one full of fresh coffee.

...What just happened? What was flicking the switch for? Was tapping the empty mug part of the procedure? Where did the coffee come from?

That's what this code is like. Why does render have a side effect of setting the value of this.isSuite? When is this.isSuite read back, in isTestPage, in includeSetupAndTeardownPages, in both, in neither? If it does get read back, why not just pass isSuite in as a Boolean? Or perhaps the caller reads it back?

Why do we return pageData.getHtml() when we never touched pageData? How did the HTML get there? Was it already there? We might make an educated guess that includeSetupAndTeardownPages has side effects on pageData, but then, what? We can't know either way until we look. And what other side effects does that have on other member variables? The uncertainty becomes so great that we suddenly have to wonder if calling isTestPage() could have side effects too.

How would you unit-test this method? Well, you can't. It's not a unit. It can't be separated from the side-effects it has on other parts of the code. (And what's up with the indentation? And where are the danged braces?)

Martin states, in this very chapter, that it makes sense to break a function down into smaller functions "if you can extract another function from it with a name that is not merely a restatement of its implementation". But then he gives us:

private boolean isTestPage() throws Exception {
  return pageData.hasAttribute("Test");
}

and:

private WikiPage findInheritedPage(String pageName) throws Exception {
  return PageCrawlerImpl.getInheritedPage(pageName, testPage);
}

and half a dozen others, none of which are even called from more than one location.

There is at least one questionable aspect of this code which isn't Martin's fault: the fact that pageData's content gets destroyed. Unlike the member variables (isSuite, testPage, newPageContent and pageCrawler), pageData is not actually ours to modify. It is originally passed in to the top-level public render methods by an external caller. The render method does a lot of work and ultimately returns a String of HTML. However, during this work, as a side effect, pageData is destructively modified (see updatePageContent). Surely it would be preferable to create a brand new PageData object with our desired modifications, and leave the original untouched? If the caller tries to use pageData for something else afterwards, they might be very surprised about what's happened to its content. But this is how the original code behaved prior to the refactoring, and the behaviour could be intentional. Martin has preserved the behaviour, though he has buried it very effectively.

Some other mild puzzles: why do we use pageData.hasAttribute("Test") to figure out whether this is a test page, but we have to consult a separate Boolean to figure out whether or not this is a test suite page? And what exactly is the separation of concerns between PageCrawler and PageCrawlerImpl, both of which are in use here?

*

Is the whole book like this?

Pretty much, yeah. Clean Code mixes together a disarming combination of strong, timeless advice and advice which is highly questionable or dated or both.

Much of the book is no longer of much use. There are multiple chapters of what are basically filler, focusing on laborious worked examples of refactoring Java code; there is a whole chapter examining the internals of JUnit. This book is from 2008, so you can imagine how relevant that is now. There's a whole chapter on formatting, which these days reduces to a single sentence: "Pick a sensible standard formatting, configure automated tools to enforce it, and then never think about this topic again."

The content focuses almost exclusively on object-oriented code, to the exclusion of other programming paradigms. Object-oriented programming was very fashionable at the time of publication. Martin is a huge proponent of OO, having invented three of the five principles which make up SOLID, and having popularised the term. But the total absence of functional programming techniques or even simple procedural code was regrettable even then, and has only grown more obvious in the years since.

The book focuses on Java code, to the exclusion of other programming languages, even other object-oriented programming languages. Java was popular at the time, and if you're writing a book like this, it makes sense to pick a single well-known language and stick with it, and Java is still very popular and may still be a strong choice for this purpose. But the book's overall use of Java is very dated.

This kind of thing is unavoidable — programming books date legendarily poorly. That's part of the reason why Clean Code was a recommended read at one time, and I now think that the pendulum is swinging back in the opposite direction.

But even for the time, even for 2008-era Java, much of the provided code is bad.

There's a chapter on unit testing. There's a lot of good, basic, stuff in this chapter, about how unit tests should be fast, independent and repeatable, about how unit tests enable more confident refactoring of source code, about how unit tests should be about as voluminous as the code under test, but strictly simpler to read and comprehend. But then he shows us a unit test with what he says is too much detail:

@Test
  public void turnOnLoTempAlarmAtThreashold() throws Exception {
    hw.setTemp(WAY_TOO_COLD);
    controller.tic();
    assertTrue(hw.heaterState());
    assertTrue(hw.blowerState());
    assertFalse(hw.coolerState());
    assertFalse(hw.hiTempAlarm());
    assertTrue(hw.loTempAlarm());
  }

and he proudly refactors it to:

@Test
  public void turnOnLoTempAlarmAtThreshold() throws Exception {
    wayTooCold();
    assertEquals(“HBchL”, hw.getState());
  }

This is done as part of an overall lesson in the virtue of inventing a new domain-specific testing language for your tests. I was left so confused by this suggestion. I would use exactly the same code to demonstrate exactly the opposite lesson. Don't do this!

Which is to say nothing of the method named wayTooCold. This is an adjective phrase. It's entirely unclear what this method does. Does it set the world's state to be way too cold? Does it react to the world's state becoming way too cold? Or is it an assertion that the world's state currently must be way too cold?

Methods should have verb or verb phrase names like postPayment, deletePage, or save. That's not me saying that. That's a direct quote from this book. Chapter 2, "Meaningful Names":

Methods should have verb or verb phrase names like postPayment, deletePage, or save.

This is perfectly sound advice. And hw.setTemp(WAY_TOO_COLD); was a perfectly unambiguous line of code. What gives?

And even if you guess correctly that calling wayTooCold() sets the temperature to be way too cold... there's no way that you could guess that it also calls controller.tic() internally. Earlier, we were advised to avoid code having side effects. This, also, was sound advice. And it is, again, being ignored in the actual written code example.

(And since we're here, this, the original unrefactored code, is a fine demonstration of the drawbacks of unadorned Booleans. What does it mean when, say, coolerState returns true? Does it mean that the cooler's current state is good, i.e. cold enough, i.e. switched off? Or does it mean that it is powered on, and actively cooling? An enum with a few values, ON and OFF, could be less ambiguous.)

*

The book presents us with the TDD loop:

First Law You may not write production code until you have written a failing unit test.

Second Law You may not write more of a unit test than is sufficient to fail, and not compiling is failing.

Third Law You may not write more production code than is sufficient to pass the currently failing test.

These three laws lock you into a cycle that is perhaps thirty seconds long. The tests and the production code are written together, with the tests just a few seconds ahead of the production code.

But the book doesn't acknowledge the missing zeroth step in the process: figuring out how to break down the programming task in front of you, so that you can take a minuscule thirty-second bite out of it. That, in many cases, is exceedingly time-consuming, and frequently obviously useless, and frequently impossible.

*

There's a whole chapter on "Objects and Data Structures". In it, we're provided with this example of a data structure:

public class Point {
  public double x;
  public double y;
}

and this example of an object (well, the interface for one):

public interface Point {
  double getX();
  double getY();
  void setCartesian(double x, double y);
  double getR();
  double getTheta();
  void setPolar(double r, double theta);
}

Martin writes:

These two examples show the difference between objects and data structures. Objects hide their data behind abstractions and expose functions that operate on that data. Data structure expose their data and have no meaningful functions. Go back and read that again. Notice the complimentary nature of the two definitions. They are virtual opposites. This difference may seem trivial, but it has far-reaching implications.

And... that's it?

Yes, you're understanding this correctly. Martin's definition of "data structure" disagrees with the definition everybody else uses. This is a very strange choice of definition, though Martin does at least define his term clearly. Drawing a clear distinction between objects as dumb data and objects as sophisticated abstractions with methods is legitimate, and useful. But it's quite glaring that there is no content in the book at all about clean coding using what most of us consider to be real data structures: arrays, linked lists, hash maps, binary trees, graphs, stacks, queues and so on. This chapter is much shorter than I expected, and contains very little information of value.

*

I'm not going to rehash all the rest of my notes. I took a lot of them, and calling out everything I perceive to be wrong with this book would be counterproductive. I'll stop with one more egregious piece of example code. This is from chapter 8, a prime number generator:

package literatePrimes;

import java.util.ArrayList;

public class PrimeGenerator {
  private static int[] primes;
  private static ArrayList<Integer> multiplesOfPrimeFactors;

  protected static int[] generate(int n) {
    primes = new int[n];
    multiplesOfPrimeFactors = new ArrayList<Integer>();
    set2AsFirstPrime();
    checkOddNumbersForSubsequentPrimes();
    return primes;
  }

  private static void set2AsFirstPrime() {
    primes[0] = 2;
    multiplesOfPrimeFactors.add(2);
  }

  private static void checkOddNumbersForSubsequentPrimes() {
    int primeIndex = 1;
    for (int candidate = 3;
         primeIndex < primes.length;
         candidate += 2) {
      if (isPrime(candidate))
        primes[primeIndex++] = candidate;
    }
  }

  private static boolean isPrime(int candidate) {
    if (isLeastRelevantMultipleOfNextLargerPrimeFactor(candidate)) {
      multiplesOfPrimeFactors.add(candidate);
      return false;
    }
    return isNotMultipleOfAnyPreviousPrimeFactor(candidate);
  }

  private static boolean
  isLeastRelevantMultipleOfNextLargerPrimeFactor(int candidate) {
    int nextLargerPrimeFactor = primes[multiplesOfPrimeFactors.size()];
    int leastRelevantMultiple = nextLargerPrimeFactor * nextLargerPrimeFactor;
    return candidate == leastRelevantMultiple;
  }

  private static boolean
  isNotMultipleOfAnyPreviousPrimeFactor(int candidate) {
    for (int n = 1; n < multiplesOfPrimeFactors.size(); n++) {
      if (isMultipleOfNthPrimeFactor(candidate, n))
        return false;
    }
    return true;
  }

  private static boolean
  isMultipleOfNthPrimeFactor(int candidate, int n) {
   return
     candidate == smallestOddNthMultipleNotLessThanCandidate(candidate, n);
  }

  private static int
  smallestOddNthMultipleNotLessThanCandidate(int candidate, int n) {
    int multiple = multiplesOfPrimeFactors.get(n);
    while (multiple < candidate)
      multiple += 2 * primes[n];
    multiplesOfPrimeFactors.set(n, multiple);
    return multiple;
  }
}

What the heck is this code? What is this algorithm? What are these method names? set2AsFirstPrime? smallestOddNthMultipleNotLessThanCandidate? Why does the code break with an out-of-bounds exception if we replace the int[] with a second ArrayList<Integer>? Earlier, we were advised that a method should be a command, which does something, or a query, which answers something, but not both. This was good advice, so why do nearly all of these methods ignore it? And what of thread safety?

Is this meant to be clean code? Is this meant to be a legible, intelligent way to search for prime numbers? Are we supposed to write code like this? And if not, why is this example in the book? And where is the "real" answer?

If this is the quality of code which this programmer produces — at his own leisure, under ideal circumstances, with none of the pressures of real production software development, as a teaching example — then why should you pay any attention at all to the rest of his book? Or to his other books?

*

I wrote this essay because I keep seeing people recommend Clean Code. I felt the need to offer an anti-recommendation.

I originally read Clean Code as part of a reading group which had been organised at work. We read about a chapter a week for thirteen weeks. (The book has seventeen chapters, we skipped some for reasons already mentioned.)

Now, you don't want a reading group to get to the end of each session with nothing but unanimous agreement. You want the book to draw out some kind of reaction from the readers, something additional to say in response. And I guess, to a certain extent, that means that the book has to either say something you disagree with, or not say everything you think it should say. On that basis, Clean Code was okay. We had good discussions. We were able to use the individual chapters as launching points for deeper discussions of actual modern practices. We talked about a great deal which was not covered in the book. We disagreed with a lot in the book.

Would I recommend this book? No. Would I recommend it as a beginner's text, with all the caveats above? No. Would I recommend it as a historical artifact, an educational snapshot of what programming best practices used to look like, way back in 2008? No, I would not.

*

So the killer question is, what book(s) would I recommend instead? I don't know. Suggestions in the comments, unless I've closed them.

Update, 2020-12-19

After suggestions from comments below, I read A Philosophy of Software Design (2018) by John Ousterhout and found it to be a much more positive experience. I would be happy to recommend it over Clean Code.

A Philosophy of Software Design is not a drop-in replacement for Clean Code. As the title suggests, it focuses more on the practice of software design at a higher level than it does on the writing or critiquing of actual individual lines of code. (As such, it contains relatively few code examples.) Because it's aimed at this higher level, I think it's possibly not a suitable read for absolute beginner programmers. A lot of this high-level theory is difficult to comprehend or put into practice until you have some real experience to compare it with. I think there actually is something of a gap in the market for an entry-level introductory programming text right now.

Having said that, I found A Philosophy of Software Design to be informative, cogent, concise and much more up-to-date. I found that I agreed with nearly all of Ousterhout's assertions and suggestions for good software design, many of which are diametrically opposed to those found in Clean Code. By virtue of providing relatively high-level advice, I feel that the book is likely to age rather better too.

Of course, software development is still moving forwards as I write this. Who knows what a good programming book will look like in ten more years?

Afterword, 2022-03-21

Probably the most common defence of Clean Code that I've seen coming from readers of this essay is that the book is still worth recommending, despite the objections above, because the advice in the book shouldn't be taken entirely literally, or applied dogmatically. The burden is on the reader to think critically, draw their own conclusions, and selectively ignore the book's advice when that advice is bad. Even the book itself says:

[M]any of the recommendations in this book are controversial. You will probably not agree with all of them. You might violently disagree with some of them. That's fine. We can't claim final authority.

I don't think this is a particularly convincing defence of the book containing bad advice and bad example code in the first place.

It's true that we should always engage critically with material instead of passively letting it wash over us. This is universally understood; this is what reading is; this is what the essay above is. This doesn't need to be stated, and it's redundant for the book itself to mention the point at all. This isn't something which can be used to shield a book from criticism.

What's more important is what the book actually says. And, especially in an instructional text like this, how carefully the book has to be read in order to get a positive experience out of it, and what happens if the book isn't read sufficiently critically.

Experienced programmers will get almost nothing out of reading Clean Code. They'll be able to weigh the advice given against their own experiences and make an informed decision — and the book will tell them almost nothing that they didn't learn years ago.

Inexperienced programmers, meanwhile — and Clean Code is an entry-level programming text, so this is the target audience, whose experiences are most important — won't be able to distinguish the good advice from the bad, and won't be able to see that the example code is bad and shouldn't be imitated. Inexperienced programmers will take those lessons at face value, and it might be years before they figure out how badly they were misled.