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 we have a reasonable definition of "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. He says DRY. This is all good advice, if a little tepid and entry-level.

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.

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 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!

This sounds a lot 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 completely 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 Java class 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.

We have two public, static methods, one private constructor and fifteen private methods. Of the fifteen private methods, fully thirteen of them either have side effects (they modify variables which were not passed into them as arguments, 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 conclude 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.

Wrong! Here's Martin's own definition! 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? It's 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 method have a side effect of setting the value of this.isSuite? When is it then 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?

Why do we return pageData.getHtml() after spending the entire method not touching pageData? 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. (And what's up with the indentation? And where are your 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 WikiPage findInheritedPage(String pageName) throws Exception {
  return PageCrawlerImpl.getInheritedPage(pageName, testPage);
}

Sidebar: There are some bad aspects of this code which aren't Martin's fault. This is a refactoring of a pre-existing piece of code, which presumably was not originally written by Martin. This code already had a questionable API and questionable behaviour, both of which are preserved in the refactoring.

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.

Second, there's 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, and extols the virtues of SOLID, to the exclusion of other programming paradigms. Object-oriented programming was very fashionable at the time of publication, but the total absence of functional programming techniques was regrettable even then, and has only grown more obvious in the years since. Opinions of SOLID have been steadily lowering in recent years too. The book focuses on Java code, to the exclusion of other programming languages, even other object-oriented programming languages. Again, 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 was probably the right call at the time, but these days there are better choices. Finally, 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 — if 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 has 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? Or does it react to the world's state becoming 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.

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

(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 are we using both int[] and ArrayList<Integer>? (Answer: because replacing the int[] with a second ArrayList<Integer> causes an out-of-bounds exception.) 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.

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?