Don't Comment Bad Code - Rewrite It

Clean Code

Many teachings preach that it is a good practice to comment your code so that your fellow programmers will know what the heck that code does or why it was written in such a way. There are many reasons why developers write code comments. A few of them sound legit. But most of them don’t.

Acceptable comments

  • Copyright or legal comments
  • Java docs ONLY for public APIs

Unacceptable comments

  • Clarification (explain what the code does)
  • Explanation of intent (explain design decisions)
  • Warning of consequences
  • Apology for an ugly hack
  • TODO comments
  • Amplification (yelling at your readers)
  • Mumbling (writing an essay or just talking to yourself)
  • Redundant comments (just repeating the code)
  • Scary noise (repeating the code but with copy-paste errors)
  • Mandated comments (by the dev process)
  • Journal comments (you have Git)
  • Closing brace comments (you have IDE)
  • Attribution and bylines (you have git blame)
  • Commented-out code (abominations - kill them on sight)
  • Non-local information (talking about code that is somewhere else)
  • Position markers
  • HTML in comments

It doesn’t take long to compare both lists. I am willing to accept one or two of those that fall under the “Unacceptable comments” category under certain circumstances. But I firmly refuse to accept writing comments as “the default”. I firmly refuse to accept the most common reason to write a comment – to explain what that code does.

A comment is a failure to express yourself in code. - Uncle Bob

The purpose of a comment is to explain the purpose of code if the code cannot explain its own purpose. Can code explain itself?

Comments that explain what the code does

Some are saying that people who advocate against comments are just lazy enough to comment their code. I would say exactly the opposite and I meant it – people that write comments to explain their code are lazy enough to think more and refactor their code to be self-explaining. Unless you are writing code in Assembler or Fortran you should not find a good reason to spend your time writing comments instead of spending your time writing good code. Modern programming languages are quite expressive if you name your classes, methods, and variables well. Writing good comments can be harder than writing good code. We are no writers. We are developers.

If your feel your code is too complex to understand without comments, your code is probably just bad.

Any time you caught yourself in writing a bunch of code and then you look at the code right after you are done with it, and think “Ah, this is awful! I’d better comment it.” No! When you write bad code, don’t comment it. Clean it! Don’t comment the code, clean the code. Comments do not make up for bad code.

Comments that explain the intent

I am far more inclined to accept comments that explain the intent or rationale behind a decision. I am far less inclined to accept comments that serve as an excuse for writing sloppy code. If you need to clarify why the code was written in a particular way you can always use a commit message. Do small meaningful commits with good commit messages and explain everything you think a future maintainer would be grateful to understand. Write a bigger commit message than the commit itself. People tend to forget. You may end up being the one that benefits the most from these descriptions. Later on, your colleagues can use git blame and read through the commits to see how the code evolved to its current state, and what were the coders thinking, what was their intent when they write it, and what technical decisions were considered, and why. It is by far better practice than polluting your source code with comments.

Make the code express your intent.

Put the effort in the code, not in the comment. Explain yourself in code, not in comments.

Comments that warn about consequences

public static Result computeSomething() {
  // Remember: Result is not thread safe!!!1!
  // Make sure you do bla bla ...
  return new Result("something");
}

You can find very questionable code even in the Java library surrounded by such comments. And if you don’t know all the hacks around that code you can get caught in chasing some really nasty concurrency issues. Well, the Java library has years of legacy to deal with. Most of the time, you don’t. Legacy in your proprietary code is your own responsibility. Pay your debt now or be forever in its chains. The Java library has billions of systems that depend on it. You, on the other side, don’t. You should be able to quickly find all the places in your code that depend on the hacky piece and adapt them to the new not-hacky piece.

Let’s have another example of warning about consequences. A comment that warns the one who dares to change the code.

# Dear maintainer:
#
# Once you are done trying to 'optimize' this routine,
# and have realized what a terrible mistake that was,
# please increment the following counter as a warning
# to the next guy:
#
# total_hours_wasted_here = 42

I often have a good laugh when reading through warning comments in code. They can be both funny and scary at the same time. Instead of taking your time to warn people just write a good test. Use a descriptive test framework to explain the weird use case behind that code. You can use BDD and Cucumber if that will explain the use cases better. Anyone can just ignore your warning and change the code. No one can change the code without changing the test. And changing old passing tests stand out in code reviews. It will need a serious justification for doing that. Do some good to this wrecked world - write a test, not a warning comment.

Comments that lie

You have seen comments in popular IDEs or editors. They stand out nicely colored in green or gray and make the code look like a piece of art. The reality is that those are all just pretty little liars, wrong misleading comments that go far beyond any truth the longer they live. Not intentionally, of course. When working in a team you get different attitudes towards comments. Some people like to write frivolous comments, others are more dogmatic writing redundant noisy comments, wannabe geeks do not write comments at all, and so on. The bigger the team the bigger the differences in attitude and opinion. If you think you can enforce through code reviews a whole team to write comments in the same way and amend those comments as the code changes - well, good luck with that! The big trouble comes when the comment says things that have nothing to do with the actual code. The problem now is that you do not know who is right and who is wrong. Is the comment right and there is a bug in the code?  Or is the code right and someone has not updated the comment? Does the comment describe a valid use case or an obsolete feature? Now you are in a situation you do not want to be in. The truth is the older the comment is the more misleading it is. It is not realistic to expect even a small team to maintain each other’s comments.

Comments silently rot. Maintaining them is expensive.

Comments that yell

// !!! HACK AHEAD
//
// The "toLowerCase()" fixes a crash with bla bla in our production system. Do
// NOT remove it!!! It will take time to design the system right and that's why
// we are doing this nasty patch.
// ...
// more bla bla
String someValue = someObject.produceSomeValue().toLowerCase();

Context is king. The toLowerCase() is trivial. It’s usually not that important. Anyone could just think “Ah, this toLowerCase() doesn’t belong here, I will just move it somewhere else.” No one would do a git blame to check the commit message and see that this toLowerCase actually fixed a production system on fire. I didn’t want people to ignore this toLowerCase so I wrote a yelling comment to make sure there are no more production crashes.

I am willing to accept comments that yell temporarily and only until the true issue is resolved. I am unwilling to accept comments that yell as part of the design, permanently. You needed to do a quick hack, maybe because your production system was on fire or some feature you’ve just released broke the whole system. Shit happens. All the time. You quick-patch the code, write a comment that yells about the hack, and push to production. And that’s ok. But what happens if your codebase is already full of comments. How do you decorate a yelling comment to stand out? If you have too many comments, everyone is just ignoring them. They are just noise. So, why not just delete them? Get rid of the noise. That way you have only the yelling comments in your code. And they yell at you every time you look at that code, forcing you to fix the issues sooner rather than later.

Commented-out code

I’ve seen commented-out code in merely every project I’ve worked on. And every single time I am going nuts about it. How am I supposed to deal with it? Is it still relevant? Leave it? Delete it? Who may ever need this snippet and when? Everyone assumes that someone else might need it so no one will delete it. At the end that code simply stays there forever and becomes less and less relevant with each day. It may even refer to collaborators that are long dead. It may be part of business logic that has changed fundamentally. It pollutes the rest of the code and distracts developers as everyone who tries to understand the rest of the class inevitably sees this commented piece and wastes time trying to understand its role. Commented-out code is evil. My advice: kill on sight. Whenever you see a commented-out code just delete it. You have your change under source control. So no need to worry about it. Anyone that might need it would grep through commits history, find it and use it.

Let’s see an example

Bad code

Try to figure out what the code in the first code block below does without reading any further. Do not figure out the arithmetic calculations. They are quite obvious. Figure out what business logic could possibly be achieved through this code.

class Calc
  def initialize(amt)
    @amt = amt
  end

  def exec
    t = if @amt <= 18200
          0
        elsif @amt <= 37000
          @amt * 0.19
        else
          @amt * 0.325
        end
    t / 12
  end
end

Bad code explained with a comment

Bad code explained with a lengthy comment is still just bad code.

class Calc
  def initialize(amt)
    @amt = amt
  end

  # Calculates the monthly income tax from the salary. Salaries fall
  # into three ranges each of which has a corresponding tax rate. If a
  # salary is below the low bracket then it is non-taxable and the tax
  # rate is zero. If the salary is between the low bracket and the high
  # bracket the tax is 0.19. If the salary is above the high bracket
  # then the tax is 0.325. The monthly income tax is calculated by
  # dividing the income tax by the number of months.
  def exec
    t = if @amt <= 18200
          0
        elsif @amt <= 37000
          @amt * 0.19
        else
          @amt * 0.325
        end
    t / 12
  end
end

Good self-explaining code

Read through the code below. I bet you can get what the business logic is far better than reading any of the previous examples.

class TaxCalculator
  LOW_INCOME = 18_200
  HIGH_INCOME = 37_000
  LOW_RATE = 0.19
  HIGH_RATE = 0.325
  NUMBER_OF_MONTHS = 12

  def initialize(annual_salary)
    @annual_salary = annual_salary
  end

  def calculate_monthly_income_tax
    annual_income_tax / NUMBER_OF_MONTHS
  end

  private

  def annual_income_tax
    @annual_salary * tax_rate
  end

  def tax_rate
    return HIGH_RATE if high_income?
    return LOW_RATE if low_income?
    0
  end

  def high_income?
    @annual_salary >= HIGH_INCOME
  end

  def low_income?
    (LOW_INCOME...HIGH_INCOME).cover? @annual_salary
  end
end

Some of the refactoring patterns used to make the code more readable are:

An explaining variable or constant is a named entity whose sole purpose is to describe the intent of that piece of code it was extracted from. Do not wait for an expression to become long and meaningless to extract it into a variable. Giving even a smaller expression a name makes the code more manageable and easier to understand.

Instead of extracting the expressions into local variables that make the methods longer, we used the Replace Temp with Query pattern to turn those temp locals into reusable methods. It’s a win-win: we have named our expressions and the method that needs them is short and clean.

When there is an expression that can be explained with a descriptive and concise name, then extract the expression into a method. Do not write a comment that explains the expression. The method name will do the same job.

Readable code increases team efficiency by a factor

Programmers spend much more time reading code than writing code. Given a feature to implement you spend a lot of time reading the current code base, thinking about how the new functionality will fit into the existing code, making sure you are not introducing any regressions, configuring your environment, manual testing, debugging, bug fixing, etc. The actual typing of the new code is a really small part of that whole process. Much smaller than reading the current source code. So make yourself a favor: write code that is easier to read. Then you will spend less time reading it when the next feature comes in. Multiply that by the number of developers in your team and you will see how many hours of reading code you have saved from your team’s capacity by writing clean readable code. It is worth spending more time when writing the code to save others time when reading the code.

Explain yourself in code

Every use of comment represents a failure. A failure to express ourselves in code.

Any fool can write code that a computer can understand. Good programmers write code that humans can understand. - Martin Fowler

You have class names, method names, variable names, argument names, constant names. So many names are there for you waiting to put them together into your poem of code.  Do not comment bad code - rewrite it. Commenting code to explain what it does means that you have failed to express yourself in code.

References

  • The Elements of Programming Style, 2nd. ed., by Kernighan and Plaugher
  • Clean Code: A Handbook of Agile Software Craftsmanship, 1st ed., by Robert C. Martin
  • Refactoring: Improving the Design of Existing Code, by Martin Fowler
  • Hunting for great names in programming by DHH