Comments

From a refactoring perspective, Martin suggests the removal of obsolete and redundant comments, and the fixing of poorly-written comments. But he somewhat reveals his hand here, in that he’s focussing very much on the ‘use knowledge of code smells to avoid making these mistakes’ angle. This is presented as guidance for writing code, rather than how to identify the code that needs refactored the most.

“If you are going to write a comment, take the time to make sure it is the best comment you can write. Choose your words carefully. Use correct grammar and punctuation. Don’t ramble. Don’t state the obvious. Be brief.”

Which is fine advice, but for some of the other advice, it’s a bit less useful.

“It is best not to write a comment that will become obsolete.”

It strikes me as such a strange thing to say. I’m assuming he must be thinking about comments that read, for example;

// Will produce an NPE until merged with branch 1234

Otherwise, how do we know the comment will be obsolete?

As for redundant comments, that’s a fun one in the context of tools like JavaDoc where the information can be consumed in code and as a compiled HTML document. I used to use a tool to generate JavaDoc comments for getters/setters. These comments were not useful, but I felt that they were needed for symmetry in the source code. It was a terrible idea. In fact, any tools that try to auto-generate documentation from source code is a terrible idea.

Functions

I don’t agree that “too many arguments” is a code smell as presented. Functions operate on data, and that data needs to come from somewhere. I agree that many function arguments is a code smell, but it also often makes sense for functions to have everything they need passed in, rather than over-using state variables. I guess what I’m saying here is that it’s more nuanced than described. I talked about my thoughts on this earlier in this series.

General

In the general section, we start to find things that are just not Code Smells. The most obvious of those is “Incorrect Behaviour at Boundaries”. I’m not really clear how this is something you can just spot in code and say “that stinks, we need to refactor that”.

Is “Overridden Safeties” a Bad Smell? I think it both is and isn’t. If you see it, it’s a bad sign, indicative of something that has gone wrong. However I’m not sure this is a pattern that can be easily spotted. I’d be surprised to see a method:

// Code is more performant if we do this
@Override
protected boolean isSafetyFeaturesEnabled(){
    return false;
}

Of course, I’m being a bit facetious. Martin specifically singles out the suppression of static analysis warnings. This is absolutely a red flag, but sometimes the linter is wrong for practical reasons that cannot be explained to a computer program. While seeing an annotation that suppresses a compiler or linter warning is certainly something to note, it is not inherently wrong.

Martin also talks about code duplication and the DRY principle (Don’t Repeat Yourself). It’s absolutely true that code duplication is a code smell that should be examined. However, I’ve found that there are some exceptions to this with non-code source files like Android Resources. I have seen layout files that attempt to serve the needs of two similar but different views. This has invariably produced more complex layout files that break in one context as soon as it’s updated for the other. I generally advise trying to apply DRY to Android’s views but not to the underlying resources. I also think that the jury is still out on the value of reuse in Android’s dimens.xml resource files as well.³

Code at the wrong level of abstraction is one of the most common things I flag up during code review. Improving this almost always results in individual classes being much simpler, even if it requires more code overall to bridge the gaps.

“Too much information” is one of the most important code smells in my opinion.

“Hide your data. Hide your utility functions. Hide your constants and your temporaries. Don’t create classes with lots of methods or lots of instance variables. Don’t create lots of protected variables and functions for your subclasses. Concentrate on keeping interfaces very tight and very small. Help keep coupling low by limiting information.”

I couldn’t agree more with this. I am a big fan of code operating on a “Need To Know” basis. Only expose what is needed. Create more abstraction if needed to prevent classes from knowing too much. For example, I have at times represented a single implementation with 2 interfaces. Given a class that passes messages around, I might create two interfaces Publisher and Subscriber, as most of the code only needs to know about one of these things. Almost nothing needs to know that there is a single class that implements both.

Martin’s comments about inconsistency hit very close to home. There will be a lot of exceptions to any design, but sometimes you find that different groups working on the same product seem to be following totally different style guides. I’ve found this is a common issue with first-party Android libraries. It’s getting better, but it’s frustrating to encounter a Java library with methods that return void and pass the function’s result back via input parameter.

Martin calls out methods that are “inappropriately” static. He states:

“In general you should prefer nonstatic methods to static methods. When in doubt, make the function nonstatic. If you really want a function to be static, make sure that there is no chance that you’ll want it to behave polymorphically.”

This is interesting because I often find myself with the opposite problem. Generalised utility functions are written as non-static member functions. Then someone else writes the same function in another class, also as a nonstatic member function. Over a codebase of sufficient size and age, the same functions crop up again and again. I think there’s a fear that it’s silly to create a utility class just for a single function. Of course, you can now test that function, and other things don’t need to be written again. Unfortunately, now you have another problem: in your sufficiently-large codebase, how do people find these utility functions?

As this section goes on, Martin starts to make increasingly outlandish claims.

“First, most people use switch statements because it’s the obvious brute force solution, not because it’s the right solution for the situation… every switch statement should be suspect.”

I don’t understand how this advice is given in a book called “Clean Code”. Switch statements are bulky, yes. However, they are really clear (ignoring fall-through weirdness) about what the code is doing. It really sounds like Martin is talking about some very specific use case, because whenever I see a switch statement we’re using switching on an enum or some other bounded value. Later, Martin states:

“For example, switch/cases with nicely named enumerations are inferior to base classes with abstract methods.”

I’m struggling to understand exactly what use case he’s talking about. The latter pattern is commonplace, so I’m struggling to visualise the terrible use-case he’s railing against.

There’s 36 ‘Smells and Heuristics’ provided in the General section. Many of these are reiterating comments made earlier in the book. Most are good advice. Some are excellent advice. Unfortunately, some are just downright strange and poorly explained.

Java

Importing only what you need seems obvious, and it’s also something most modern IDEs will just handle for you. It’s not bad advice though. Martin tells us not to inherit constants, which is good advice. Perhaps the most useful bit of advice in this section is about enums. As enums are classes, they can contain code as well, which is something I’ve taken advantage of plenty of times in my career. My only complaint here is this: is this a code smell or a heuristic?

Names

This section has a lot of good advice. I particularly liked “Use Long Names for Long Scopes”, which is one of those things that I’ve been doing the whole time but is nice to see written down. I’m less sure about the last rule here, “Names Should describe Side-Effects”.

“This function does a bit more than get an ‘oos’; it creates the ‘oos’ if it hasn’t been created already. Thus, a better name might be createOrReturnOos.”

So it’s a public method that lazilly initializes a given object. Depending on the use case, do we really want to expose the underlying mechanism? What does the caller need to know? If it’s null we create it, sure, but what’s the use case here? Is there another method called getOss() that doesn’t do the lazy bit? I doubt it, because that’s the original name of the method.

This seems to go against many of the recurring arguments made in this book about levels of abstraction, functions only doing one thing and so on. I think it’s probably just a bad example, to be honest. I think better advice should be that functions should not have side-effects.⁴

Tests

The tests section re-treads familiar ground, but as with many of the other guidelines here, we are pretty firmly in the territory of “how to write better code”. Not that any of it is bad advice, of course.