Clean Code : Comments

[Tim Cooke]

I'd like to talk about Clean Code Chapter 4: Comments.

On page 54:

Clean Code wrote:Every time you express yourself in code, you should pat yourself on the back. Every time you write a comment, you should grimace and feel the failure of your ability of expression.

Uncle Bob is far more opinionated about this in person than he appears in his book, he really means it..... I know plenty of programmers, decent programmers, who love comments and write them all over the place, in my view for no good reason a lot of the time. I would work on writing as little comments as possible in my code and just try to write clean expressive code. One of the examples he gives on this is "Which would you rather see? This:"

// Check to see if the employee is eligible for full benefits
if ((employee.flags & HOURLY_FLAG) &&
    (employee.age > 65))

"Or this?"

if (employee.isEligibleForFullBenefits())

It's a fairly trivial example but I see similar code to this all the time.

Are you pro comments? Or anti comments? Let's discuss.

[Campbell Ritchie]

There is a design question there. Where are the rules which decide whether the Employee is eligible? Are they inside the Employee class? In which case the Employee class may be too complicated, but you are complying with the “S” bit of “SOLID”. Or are they defined in the benefits class? In which case the Benefits class needs the “S” bit. If the latter, then maybe this is the correct answer

// Eligible for full benefits if flags and hourly and over 65
if ((employee.flags & HOURLY_FLAG) &&
    (employee.age > 65))

Forgetting about the encapsulation whereby Employees have non‑private ages
If the rules are in the Employee class, then the isEligibleEtc method is the way to go.

[Campbell Ritchie]

Why is he mixing & and &&?

[Ron McLeod]

Campbell Ritchie wrote:Why is he mixing & and &&?

He is performing a bitwise operation using HOURLY_FLAG as a mask to get the bit representing if the employee is an hourly worker or not.

[Matthew Brown]

Campbell Ritchie wrote:There is a design question there. Where are the rules which decide whether the Employee is eligible? Are they inside the Employee class?. ... Or are they defined in the benefits class?

I think the argument (which I tend to agree with, even if I don't always practice it) is that it should be a separate method regardless. So by all means put the rules in the Benefits class* if that's the right place for it to be, but then you have:

if (isEligibleForFullBenefits(employee))

* Or even farm it out into a separate class if you're using the Strategy pattern or something similar.

[Campbell Ritchie]

Ron McLeod wrote: . . . He is performing a bitwise operation using HOURLY_FLAG as a mask to get the bit . . .

No, he isn't, because otherwise it would fail to compile when combined with the && operator which requires a boolean on both sides.

[Campbell Ritchie]

Well, && would work in C/C++ but not in Java.

[Ron McLeod]

Right - because you cannot convert an int to a boolean. I guess something like this would work.

if (((employee.flags & HOURLY_FLAG) > 0) && (employee.age > 65))

[Campbell Ritchie]

That should work, yes

[Jeanne Boyarsky]

While I wouldn't say I never use comments, I do reserve them for special cases and documentation. In your example, I'd use the method.

I use comments for expressing things like:
1) We tried X and it blew up.
2) Workaround for the bug in external product X - remove when they fix it.
3) Referencing specific things the user said, file formats, etc.

One time the user asked for "proof" a bug was fixed. I sent him the class that has his name in the comment about the discussion. He was entertained.

[Junilu Lacar]

Here's the progression of the quality of comments, IMO:

// ---------
// Poor (avoid) - HOW the code does what it does

/*
 * Iterates through ArrayList and adds each element's foo to a total
 */

// ---------
// Better - WHAT the code does

/*
 * Calculates the sum of all foo in a collection 
 */


// --------
// Even better - no comment, just intention-revealing name

int sumOfAllFoo(Collection stuff);

If I do feel a need to add a comment, it's usually to explain WHY something is done the way it's done and those comments most likely get related (using @see or @link) to test cases I've written while doing TDD. These comments are still a bit brittle in that you can easily forget to update the comment when you update the tests or refactor the design some more but you just need a bit of discipline to keep a handle on that.

My other guideline for my developers is to make any comment resilient to change. If your comment has to be updated because you refactored the code, then it's too brittle and needs to be reworded such that the description of the behavior or motivation doesn't change, no matter what changes to the implementation details are made.