However, for me the main problem with this book is that many of the code examples in it are just awful .
In the third chapter, “Functions,” Martin gives various tips for writing good features. Probably the strongest advice in this chapter is that functions should not mix levels of abstraction; they should not be performing high-level or low-level tasks because this confuses and confuses the responsibility of the function. There are other important things in this chapter: Martin says that function names must be descriptive and consistent, and must be verb phrases, and must be carefully selected. He says that functions should only do one thing, and do it well. He says functions shouldn't have side effects (and he gives a really great example) and that output arguments should be avoided in favor of return values. He says that functions should usually be either commands that do something,or by requests that for somethinganswer , but not both at once. He explains to DRY . These are all good tips, although a bit superficial and entry-level.
But there are more dubious statements in this chapter. Martin says that the arguments of the logical flag are bad practice, which I agree with, because the unvarnished ones
trueor falsein the source code are opaque and unclear compared to the explicit ones IS_SUITEor IS_NOT_SUITE... but Martin’s reasoning is rather that the logical argument means that the function does more, than one thing that she should not do.
Martin says that it should be possible to read one source file from top to bottom as a story, with the level of abstraction in each function decreasing as it reads, and each function goes down the other. This is far from universal. Many source files, I would even say most source files, cannot be neatly hierarchical this way. And even for those we can, the IDE allows us to trivially jump from function call to function implementation and back, just like we browse websites. Also, are we still reading code from top to bottom? Well, maybe some of us do that.
And then it gets weird. Martin says that functions should not be large enough to contain nested structures (conventions and loops); they should not be indented more than two levels. He says that blocks should be one line long, probably consisting of one function call. He says that an ideal function has no arguments (but still no side effects ??), and that a function with three arguments is confusing and difficult to test. The strangest thing is that Martin claims that the ideal function is two or four lines of code . This advice is actually placed at the beginning of the chapter. This is the first and most important rule:
: . : . . , , . , . 3000 . 100 300 . 20 30 . ( ), .
[...]
When Kent showed me the code, it struck me how compact all the features were. Many of my functions in Swing programs were stretched vertically for almost kilometers. However, each function in Kent's program was only two, three, or four lines long. All the features were pretty obvious. Each function told its own story, and each story naturally led you to the beginning of the next story. That's how short the functions should be!
This entire tip concludes with a source code listing at the end of Chapter 3. This code example is Martin's preferred refactoring of the Java class that comes from the 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");
}
}Again, this is Martin's own code, written according to his personal standards. This is the ideal presented to us as a training example.
At this point, I confess that my Java skills are outdated and rusted, almost as outdated and rusted as this book, which came out in 2008. But even in 2008, this code was illegible garbage?
Let's ignore
importthe wildcards.
We have two public static methods, one private constructor and fifteen private methods. Of the fifteen private methods, as many as thirteen either have side effects (they change variables that were not passed to them as arguments, for example
buildIncludeDirective, which has side effects onnewPageContent), or call other methods that have side effects (for example include, which calls buildIncludeDirective). Only isTestPageand findInheritedPagelooks like without the side effects. They still use variables that are not passed to them ( pageDataand thus testPage), but they seem to do so without side effects.
At this point, you can conclude that perhaps Martin’s definition of “side effect” does not include the member variables of the object whose method we just called. If we accept this definition, then five of these variables,
pageData, isSuite, testPage, newPageContentandpageCrawlerare implicitly passed to each call to the private method, and this is considered normal; any private method is free to do whatever it likes with any of these variables.
But this is a wrong assumption! Here's Martin's own definition from an earlier part of this chapter:
Side effects are a lie. Your function promises to do one thing, but does something different, hidden from the user . Sometimes it makes unexpected changes to the variables of its class - say, assigns them to the values ​​of parameters passed to the function or global system variables. In any case, such a function is an insidious and malicious lie, which often leads to the creation of unnatural time bindings and other dependencies.
I love this definition! I agree with this definition! This is a very useful definition! I agree that it is 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 is incredibly difficult to understand what any of these codes does, because all 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 the side effect of setting the value
this.isSuite? Why not just pass it isSuiteas a boolean to later method calls? Why are we returning pageData.getHtml()after spending three lines of code doing nothing with pageData? We could make an includeSetupAndTeardownPageseducated guess as to what has side effects on pageData, but then what? We cannot know either one or the other until we look. And what other side effects does this have on other member variables? Uncertainty increases so much that we suddenly wonder if it can isTestPagealso have side effects. (And what is this ledge? And where are the braces?)
Martin argues 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 just a repetition of its implementation." But then he gives us:
private WikiPage findInheritedPage(String pageName) throws Exception {
return PageCrawlerImpl.getInheritedPage(pageName, testPage);
}Note: some of the bad aspects of this code are not Martin's fault. This is a refactoring of an already existing piece of code that, apparently, was not originally written by him. This code already had dubious APIs and dubious behavior, both of which are stored in refactoring. Firstly, the class name is
SetupTeardownIncluderterrible. It's at least a nominal phrase, like all class names, but it's a classic suffocated verb phrase. This is the class name that you always get when you work in strictly object-oriented code, where everything should be a class, but sometimes you really need just one simple function.
Second, the content is
pageDatadestroyed. Unlike the member variables ( isSuite, testPage, newPageContentand pageCrawler), we actually do not ownpageDatafor change. It is initially passed to the top-level public renderers by the external caller. The renderer does a great job and ultimately returns an HTML string. However, during this work, as a side effect, it is pageDatadestructively modified (see updatePageContent). Of course, would it be preferable to create a completely new object PageDatawith our desired modifications and leave the original untouched? If the caller tries to use it pageDatafor something else, he may be very surprised at what happened to its contents. But this is exactly how the original code behaved before Martin's refactoring. He retained this behavior, although he buried it very effectively.
*
Is the whole book like that?
Mostly yes. Clean Code mixes a disarming combination of strong, timeless tips and advice that is highly questionable or out of date. The book focuses almost exclusively on object-oriented code and calls for the virtues of SOLID, excluding other programming paradigms. It focuses on Java code, excluding other programming languages, even other object oriented languages. There is a chapter on Smells and Heuristics, which is nothing more than a list of pretty reasonable clues to look for in your code. But there are a few mostly empty chapters that focus on time-consuming and well-tried examples of refactoring Java code. There is a whole chapter studying the internal components of JUnit (the book was written in 2008, so you can imagine how relevant this is now). The general use of Java in the book is very outdated. This kind of thing is inevitable - programming books are traditionally outdated quickly - but even for that time, the code provided is bad.
There is a chapter on unit testing. This chapter has a lot of good - albeit basic - information about how unit tests should be fast, independent, and reproducible, how unit tests allow more confident refactoring of source code, and that unit tests should be about the same lengthy like the code under test, but much easier to read and understand. Then the author shows a unit test, where, according to him, there are too many details:
@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 proudly remakes it:
@Test
public void turnOnLoTempAlarmAtThreshold() throws Exception {
wayTooCold();
assertEquals(“HBchL”, hw.getState());
}This is done as part of a general lesson on the value of inventing a new testing language for a specific area for your tests . I was so confused by this statement. I would use exactly the same code to demonstrate the exact opposite advice! Do not do that!
*
The author presents three laws of TDD:
. , .
. , . .
. , .
, , , 30 . , .
… But Martin is oblivious to the fact that breaking down programming tasks into tiny thirty-second chunks is in most cases insanely time consuming, often obviously useless, and often impossible.
*
There is a chapter "Objects and Data Structures", where the author gives an example of a data structure:
public class Point {
public double x;
public double y;
}and this example of an object (well, an interface for one object):
public interface Point {
double getX();
double getY();
void setCartesian(double x, double y);
double getR();
double getTheta();
void setPolar(double r, double theta);
}He's writing:
, . , . . . , , . , .
And it's all?
Yes, you got it right. Martin's definition of “data structure” is at odds with the definition everyone else uses! The book does n't say anything at all about pure coding using what most of us think of as data structures. This chapter is much shorter than you might expect and contains very little useful information.
*
I'm not going to rewrite all the rest of my notes. I have too many of them, and it would take too long to list everything that I think is wrong in this book. I will focus on another egregious code example. This is the prime number generator from Chapter 8:
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 is this code? What are the names of the methods?
set2AsFirstPrime? smallestOddNthMultipleNotLessThanCandidate? Should it be clean code? A clear, intelligent way to sift through primes?
If that is the quality of the code this programmer creates - at his leisure, under ideal conditions, without the pressure of actual production software development - then why bother paying attention to the rest of his book at all? Or his other books?
*
I wrote this essay because I constantly see people recommending “Clean Code”. I felt the need to offer an anti-recommendation.
I originally read Clean Code in a group at work. We read a chapter a week for thirteen weeks.
Now, you do not want the group to express only unanimous agreement at the end of each session. You want the book to evoke some kind of reaction from readers, some additional comments. And I guess that to a certain extent this means that the book should either say something that you disagree with, or not cover the topic in full, as you feel it should. Based on this, the "Clean Code" turned out to be suitable. We had good discussions. We were able to use individual chapters as a starting point for a deeper discussion of current contemporary practices. We talked about many things that were not covered in the book. We disagreed in many ways.
Would I recommend this book to you? Not. Even as a text for beginners, even with all the caveats above? Not. Maybe in 2008 I would recommend this book to you? Can I recommend it now as a historical artifact, an educational snapshot of what the best programming practices looked like back in 2008? No, I wouldn’t.
*
So, the main question is which book (s) would I recommend instead? I dont know. Suggest in the comments, unless I closed them.