I spent a mystifying half hour chasing down a bug recently, so I thought I would share.

Here is a simple scheduled executor:

public class Exec {
    final static Logger logger = LoggerFactory.getLogger(Exec.class);
    private static final ScheduledExecutorService executor =
        Executors.newSingleThreadScheduledExecutor(
            new ThreadFactory() {
                @Override
                public Thread newThread(Runnable r) {
                    Thread result = new Thread();
                    result.setName("BuggyExecutor");
                    return result;
                }
            });
    public static void main(String[] args) {
        executor.scheduleAtFixedRate(() -> logger.info("Tick"),
                0,
                1, TimeUnit.SECONDS);
    }
}

I always use a ThreadFactory in my executors since seeing thread names plainly in your trace simplifies debugging threading issues considerably. Whenever I see a pool-2-thread-3 in my thread dump, I track down the lazy library that caused that monstrosity and I seriously consider replacing it with one that is written by developers more respectful of my time.

Other than that, this code is pretty straightforward and if you run it, you would expect the string “Tick” to be displayed every second:

17:37:18.780 [BuggyExecutor] INFO  com.beust.Exec - Tick
17:37:19.780 [BuggyExecutor] INFO  com.beust.Exec - Tick
17:37:20.780 [BuggyExecutor] INFO  com.beust.Exec - Tick

However, if you run the code as provided above, you will see that it does absolutely nothing.

I recently reviewed a commit that said “Fix the list view bug”. I reviewed it, saw that it was fixing an off-by-one error, approved it and moved on.

A few days later, another commit went by that said “Really fix the list view bug”. This fix was a bit more involved and caused the first item in the list view to sometimes receive the wrong styling. I then realized that I shouldn’t have approved the first commit without asking a few more questions.

Here is another scenario. Let’s say you are asked to review the following code:

public static int compare(@Nonnull Long a, @Nonnull Long b) {
	return a.compareTo(b);
}

Seems pretty harmless, doesn’t it? No reason not to approve it.

How about this one:

/**
 * @return 0 if the two numbers are equal, 1 otherwise.
 */
public int cmp(@Nonnull Long a, @Nonnull Long b) {
	return a.compareTo(b);
}

Now we have a problem: the code and the comment do not agree. This should not be approved before asking the developer to fix this (either change the comment or change the code).

There is this prevalent notion in the software world that good code doesn’t need comments, that it stands on its own. Or that comments are a code smell.

Irritatingly, this myth just won’t die despite repeated evidence that comments are sometimes vital to code correctness. Proponents of this myth point out that it’s easy for comments to get out of sync with the code (see the example above) and decide that because this approach is not perfect, it should be avoided altogether.

This is a false dichotomy that is easily avoided by making it clear to your teammates that both code and comments need to be reviewed.

The problem with the first example that I gave is that the developer failed to disclose what the intent of his code was. The code type checks, is correct and fixes a bug, but it turns out to be doing something different than the developer intended, and the reviewer would have caught it if the developer had explained what the intent of his code is.

Not all code needs comments, but certain pieces of code are useless and can’t be verified without comments.

Your code says “What?”, your comments say “Why?”. Sometimes, you need both in order to assess the correctness of a commit. Just make sure you review comments as seriously as you review code.