Stop Commenting Your Code – You’re Just Confusing Things!

In my post on reading other people’s code, I mentioned my views on the pointlessness of comments in code. It seemed worthwhile to expand on this, since I got such a wide variety of responses.

And yes, in the main I am going to suggest you don’t bother commenting most of your code. Unfortunately for the lazy among us, there something else you should use to replace those comments … but we’ll get to that later.

An Example

Here’s an example Java class, our wonderful class WorldDomination.java:

package com.designbygravity; import java.util.*; /** * This class is designed to enable world domination of a * certain world. * * You are allowed to dominate a world in several ways, * economically, politically, militarily, or by invoking the * power of His Noodly Majesty, the Flying Spaghetti Monster. * * The design allows for query the current state, and for a * world to be dominated multiple times. * * You can also release world from your tryanny. * * No hashCode()/equals support. * * Thread-safe, as much as the call to Random.nextInt(...) * is thread-safe. * * It would be nicer if there was an enum class for the * types of domination; perhaps a complete member class * describing types of domination, time to dominate, * etc. * * @author cschanck * */ public class WorldDomination { private String worldName; private volatile boolean domEcon = false; private volatile boolean domPol = false; private volatile boolean domMil = false; private volatile boolean domNoodle = false; private Random rand; public WorldDomination(String worldName) { this.worldName = worldName; rand = new Random(worldName.hashCode()); } @Override public String toString() { return "WorldDomination [" + worldName + " domEcon=" + domEcon + ", domMil=" + domMil + ", domPol=" + domPol + ", domNoodle=" + domNoodle + "]"; } public String getWorldName() { // return the world name return worldName; } /** * Dominate, waiting some amount of time, emitting status messages ever * interval. */ private void dominate(int max, String units, String msg) { max=Math.max(max,1); int yrs = rand.nextInt(max) + 1; for (int i = max; i >0; i--) { System.out.println(i + " " + units + " " + msg); delayAwhile(100); } System.out.println("Dominated!"); } private void delayAwhile(int i) { try { Thread.sleep(i); } catch (InterruptedException e) { ; } } // ---------------- // status methods // ---------------- public boolean isDominated() { return isDominatedEconomically() || isDominatedPolitically() || isDominatedMilitarily(); } public boolean isDominatedEconomically() { return domEcon; } public boolean isDominatedPolitically() { return domPol; } public boolean isDominatedMilitarily() { return domMil; } public boolean isDominatedNoodly() { return domNoodle; } // ------------------ // domination methods // ------------------ public void dominateEconomically() { dominate(50, "years", "until economic domination"); this.domEcon = true; } public void dominatePolitically() { dominate(16, "years", "until political domination"); this.domPol = true; } public void dominateMilitarily() { dominate(2, "years", "until military domination"); this.domMil = true; } public void dominateNoodly() { // dominate in a noodly way dominate(0, "years", "until Noodly domination"); this.domNoodle = true; } public void freeWorld() { // set them to false this.domEcon = this.domMil = this.domPol = this.domNoodle = false; } }

First, let’s divide comments into two types, the Possibly Worthwhile and the Probably a Waste.

Possibly Worthwhile Comments
These comments are comments that cover computational units larger than a statement. Comments for a class or a method fall into this category. They are not terribly concerned with documenting the nitty-gritty of the code.

In the above example, we find class comments. These comments talk about what the goals of the class are, what the gotchas in this implementation might be, etc. More importantly, they document what the intent of the class was when it was written — the mental model the writer had in mind when he or she wrote it. This is important; breadcrumbs for the future.

Some comments, like those before the isDominated() method, exist just to group methods together. OK, but we use structured editors so much these comments are more like scroll markers than anything informational. Still, not harmful.

Finally, there’s one method that actually does something: dominate(int max, String units, String msg); all the comment here does is give a quick thumbnail of the idea. Might be useful, might drift out of sync.

This example class doesn’t run to complex data structures, but if you have them you should document a bit about how they work. Really. Data still structures programs, after all.

Probably a Waste
For this example, I sprinkled in some comments that I would consider a waste of time. Look at the following methods:

• getWorldName()
• dominateNoodly()
• freeWorld()

Each is kind enough to document an operation that is self-evident with a little effort. If you spend a minute, and read the code, you could write the comment yourself. But these three comments are pointless, because even though they’re present, even if the code was checked in yesterday, you’re going to have to read the code and understand it anyway, because the comment could be wrong. So … if you are going to read the code anyway, why bother with the comment? It’s just noise. A professional doesn’t need the comment.

So those three comments potentially do more harm than good. Give it up. Don’t bother. I might delete them if I saw them. Your call.

So What Should You Do?
In a prior post, I derided design docs, specifications, requirements, etc. as far too likely to be wrong to be helpful, once the code has been written.

So what can you do so that those who follow you into this code can understand it?

Simple: Tests. Yep, as my nine-year old would say, “I went there.”

Now, I’m not going to lecture you on Test Driven Development. Not going to tell you that writing tests before code will make your code magically delicious. Nope, not going to evangelize that line here. Some other day perhaps. I’ll just walk away. (And I use the term Unit Test loosely; any repeatable, automated test framework will do.)

But, if you really want to leave an accurate description of how your code is intended to behave, write a test for it. Seriously. You give me a chunk of code, and a set of tests against it, and I can be pretty sure of being able to at least winkle an understanding of the code. Even more importantly, the tests provide a workbench to try out different behaviors, new behaviors. Given tests, I can write tests for cases no one originally thought of, cases I’m interested in. And tests, assuming some automated framework for running them, don’t drift out of spec.

If they do break, you fix them.

Or, ahem, you remove the test. In which case someone should whack you on the head with a large stick. I’m just saying.

(Another construct that is very useful is assertions; they let you embedded what amounts to tiny tests into your code. They can be great, but too many of them can obscure the code, and it is often problematic to write assertions with complex test cases without introducing side effects. If you like assertions, go for it.)

A Companion Test

Here is a JUnit test I wrote for the above class:

package com.designbygravity; import junit.framework.*; public class WorldDominationTest extends TestCase { private static final String GALLIFREY = "Gallifrey"; public void testCreation() { WorldDomination world=new WorldDomination(GALLIFREY); assertTrue(world.getWorldName().equals(GALLIFREY)); assertTrue(world.isDominated()==false); } public void testDominateEconomically() { WorldDomination world=new WorldDomination(GALLIFREY); assertTrue(world.isDominated()==false); world.dominateEconomically(); assertTrue(world.isDominated()==true); assertTrue(world.isDominatedEconomically()==true); assertTrue(world.isDominatedMilitarily()==false); assertTrue(world.isDominatedNoodly()==false); assertTrue(world.isDominatedPolitically()==false); world.freeWorld(); assertTrue(world.isDominated()==false); assertTrue(world.isDominatedEconomically()==false); assertTrue(world.isDominatedMilitarily()==false); assertTrue(world.isDominatedNoodly()==false); assertTrue(world.isDominatedPolitically()==false); } public void testDominatePolitically() { WorldDomination world=new WorldDomination(GALLIFREY); assertTrue(world.isDominated()==false); world.dominatePolitically(); assertTrue(world.isDominatedEconomically()==false); assertTrue(world.isDominatedMilitarily()==false); assertTrue(world.isDominatedNoodly()==false); assertTrue(world.isDominatedPolitically()==true); world.freeWorld(); assertTrue(world.isDominated()==false); assertTrue(world.isDominatedEconomically()==false); assertTrue(world.isDominatedMilitarily()==false); assertTrue(world.isDominatedNoodly()==false); assertTrue(world.isDominatedPolitically()==false); } public void testDominateMilitarily() { WorldDomination world=new WorldDomination(GALLIFREY); assertTrue(world.isDominated()==false); world.dominateMilitarily(); assertTrue(world.isDominatedEconomically()==false); assertTrue(world.isDominatedMilitarily()==true); assertTrue(world.isDominatedNoodly()==false); assertTrue(world.isDominatedPolitically()==false); world.freeWorld(); assertTrue(world.isDominated()==false); assertTrue(world.isDominatedEconomically()==false); assertTrue(world.isDominatedMilitarily()==false); assertTrue(world.isDominatedNoodly()==false); assertTrue(world.isDominatedPolitically()==false); } public void testDominateNoodly() { WorldDomination world=new WorldDomination(GALLIFREY); assertTrue(world.isDominated()==false); world.dominateNoodly(); assertTrue(world.isDominatedEconomically()==false); assertTrue(world.isDominatedMilitarily()==false); assertTrue(world.isDominatedNoodly()==true); assertTrue(world.isDominatedPolitically()==false); world.freeWorld(); assertTrue(world.isDominated()==false); assertTrue(world.isDominatedEconomically()==false); assertTrue(world.isDominatedMilitarily()==false); assertTrue(world.isDominatedNoodly()==false); assertTrue(world.isDominatedPolitically()==false); } }

It’s a pretty simple test. But it documents some useful things. For example, nowhere in the test does it test concurrent usage; if you knew the class was being used concurrently, and things weren’t working, that might be a fruitful avenue to explore. It doesn’t dominate, free, then dominate a world again. Maybe you need that, and that breaks?

Write a new test. My rule of thumb is pretty simple, and (I suspect) pretty commonly-held: if a feature or usage pattern isn’t tested, don’t believe that feature works. So write some tests!

In the end, comments only exist to help someone else understand your code. If a comment doesn’t contribute to that end, feel free to toss it. And if you really want to help others understand your code, write tests.

Tests. A much better documentation than comments.

Explore posts in the same categories: Tech

Tags: code comments, Design By Gravity, dislikes, Software Development

You can comment below, or link to this permanent URL from your own site.