It's probably time to stop recommending Clean Code

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 opinion 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. 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. Outside of a book, do we still read code from top to bottom? Well, maybe some of us do.

And then it gets weird. Martin says that functions should not be large enough to hold nested structures (conditionals and loops); 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 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!

All of this advice culminates in the following source code listing at the end of the 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 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:

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! 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();
}

Why does this method have a side effect of setting the value of this.isSuite ? Why not just pass isSuite as a Boolean to the later method calls? Why do we return pageData.getHtml() after spending three lines of code not doing anything to 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 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 classic strangled nouned verb phrase. It's the 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, they might be very surprised about what's happened to its content. But this is how the original code behaved prior to Martin's refactoring. He has preserved the behaviour. Although, he has buried it very effectively.

*

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. The book focuses almost exclusively on object-oriented code and exhorts the virtues of SOLID , to the exclusion of other programming paradigms. It focuses on Java code, to the exclusion of other programming languages, even other object-oriented programming languages. There is a chapter on "Smells and Heuristics" which is little more than a bullet pointed list of fairly reasonable signs to look out for in code. But 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.) The book's overall use of Java is very dated. This kind of thing is unavoidable — programming books date legendarily poorly — but even for the time, 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 value of inventing a new domain-specific testing language for your tests . I was left so confused by this assertion. I would use exactly the same code to demonstrate exactly the opposite lesson! Don't do this!

*

He 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 he doesn't address the fact that breaking programming tasks down into minuscule thirty-second bites is in most cases insanely time-consuming, and frequently obviously useless, and frequently impossible.

*

There's a chapter "Objects and Data Structures", where he provides 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);
}

He 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?

Yeah, you're understanding this correctly. Martin's definition of "data structure" disagrees with the definition everybody else uses! There is no content in the book at all about clean coding using what most of us consider to be data structures . This chapter is much shorter than you'd expect and contains very little useful information.

*

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 take way too long. 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 are these method names ? set2AsFirstPrime ? smallestOddNthMultipleNotLessThanCandidate ? Is this meant to be clean code? Is this meant to be a legible, intelligent way to sieve for prime numbers?

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 — 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.

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. Even as a beginner's text, even with all the caveats above? No. In 2008, would I have recommended this book? Would I recommend it now 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.