Bad comments can be a little bit of a problem, even if they appear harmless.

In some projects (most?) comments multiply like tribbles creeping up everywhere. Sometimes the comments are especially useful but other times comments useless are destructive to maintainability. Contrary to some beliefs, bad and inaccurate comments are not mostly harmless.

Bad comments take many forms. There are four forms that I think could be considered anti-patterns:

1. Programming 101 comments

Consider the following code snippet:

// Print "Hello World" to
// standard output without
// a new line.
System.out.print("Hello World");

No doubt this sort of commenting is useful to students in a class, but in the real world even non-Java programmers can figure this one out. Programming 101 comments are most commonly used by students and recent graduates after learning a new language feature or library.

2. Stating the obvious comments

Consider the following code snippet:

// can use express checkout
if (shoppingCart.getItems().size() > 10)
{
System.out.println("Please be courteous; don't use express checkout.");
}

Consider stating the obvious comments as a code smell. In fact we can improve this method by making it self-documenting. Instead of writing a comment, make the code say it instead:

public static final int MAX_EXPRESS_CHECKOUT_ITEMS = 10;

if (canUseExpressCheckout(shoppingCart))
{
System.out.println("Please be courteous; don't use express checkout.");
}

public canUseExpressCheckout(ShoppingCart shoppingCart)
{
return shoppingCart.getItems.size() <= MAX_EXPRESS_CHECKOUT_ITEMS;
}

3. Comment Me! comments

“Comment Me!” comments are the lazy man’s way of commenting. If you’re getting paid by the number of lines or the number of comments, than this type of commenting is great. Otherwise, it’s a waste of your time to put them and a waste of time for the person who has to read this junk.

Don't let your code look like this.

4. Code memorial comments

Code memorial comments proclaim “The code is dead. Long live the dead code!” Not exactly the kind of proclamation any new king would like to hear.

Rather than letting bad or good code die and receive a proper burial in your favorite code repository, these comments are embalmed and enshrined for all to see. When old code is enshrined, it takes away precious screen real estate from the living code.

I’ve heard several defenses of this anti-pattern. Requirements keep changing, we’ll just have to put it back. Make it easy to put it back.

I suspect the reason for holding onto dead code is deeper than any of that. Imagine spending hours or days working on a problem. No doubt it can be emotional to throw away your hard work.

Delete the old code anyhow. The new found cleanliness will be extremely liberating. Besides, you can always lay flowers by its grave in your version control system’s history.

5. I WAS HERE comments

Comments can take on the form of being a replacement for version control features (like CVS’ annotate command). These can take on forms similar to this:

// ARL 010109 begin
... some code here ...
// ARL 010109 end

There are several problems with I WAS HERE comments. First, it clutters code unnecessarily. Second, it duplicates functionality that already exists in version control systems. Third, it’s error prone.

the cake is a lie.

The comment is a lie.

Comments always lie. If not now, they will soon. Often code changes but the comments do not. I have seen countless cases of comments that lied literally the second they were committed.

Bad/incorrect documentation is worse than no documentation. I suppose Stack Overflow isn’t exactly the best place to a consensus, but the comments and stories are interesting. In my not-so-humble opinion, misleading documentation – especially that that appears authoritative – really sucks.

Striving to make clean code, simple code (low cyclomatic complexity) and self-documenting code (good class, method, variable names and unit tests) do wonders to lessen the need for comments.

When it comes to unit testing you might find yourself wanting to test private methods. Here’s four solutions, some much better than others.

1. Don’t test private methods (refactor!)

If you find yourself needing to test private methods, you’re code is trying to tell you something – listen up!

Unit tests should test the behavior of classes and not the implementation details. Unit tests should be able to naturally cover your private methods. If you find this difficult or impossible to do, consider it a code smell.

Testing implementation will become a barrier to refactoring since you will be unable to change the implementation without updating tests. As Charles Miller put it “The amount of work you have to do to improve your code becomes the amount needed to change the private methods, _plus_ that required to change the tests. As such, you’re less likely to make the improvement.”

If you choose to test private methods keep in mind you are likely taking on some technical debt.

Pros: You’re not testing private methods. Best solution. Results in better design.
Cons: Can sometimes be tricky to implement, especially in legacy applications.

2. Use reflection

Thanks to reflection, access levels are more of a suggestion than a requirement. With a little bit of code, we can access any private method (or field for that matter).

First use reflection to change the access level:

MySuperCoolDao mySuperCoolDao = new MySuperCoolDao();
// get method by name "isThisCool" and signature
Method isThisCoolMethod = mySuperCoolDao.getClass().
getDeclaredMethod( "isThisCool", String.class );
// make method accessible
getCoordiantesMethod.setAccessible( true );

Then we use the newly accessible method:

boolean isWindowsCool = isThisCoolMethod.invoke(mySuperCoolDao, "Windows");
boolean isUbuntuCool = isThisCoolMethod.invoke(mySuperCoolDao, "Ubuntu");

assertFalse("Windows is not cool...", isWindowsCool);
assertTrue("Ubuntu is very cool", isUbuntuCool);

A big negative to this method is maintainability. In fact this just happened to me today: I refactored out a private method and as expected the test failed. Unlike other solutions (like the package level solution below), the failure was at runtime – not compile time.

Pros: Relatively easy and quick. Does not break encapsulation.
Cons: Tests become busier. More difficult to keep tests updated with production code. Security settings could prevent this technique from working.

3. Use package level access

This solution is very easy, simply remove private to get the (default) package level access. As long as your tests are in the same package as the production code, no problem. Since it’s good practice for your unit tests to be in the same package (but in a separate source folder), this works well.

Right out of college, this was the first way I learned to handle these situations. Almost everything method was package level. It never really smelled quite right, but the postives out weighed the negatives. In hindsight it might not have been the best solution, but there are far worse things.

Pros: Quick and easy.
Cons: Breaks encapsulation. Classes become a little noisier (although not as bad as using public).

4. Mix production and test code

This is the worst of all the solutions, so I hesitate to even bring it up. By making your test class an inner class in your production code, your tests will have access to private methods. Unfortunately this solution leaves your production code dependent on your test code.

Challenge: is there a legitimate reason to do this? I can’t think of one.

Pros: At least you’re writing tests! Weaksauce, I know.
Cons: Production code contains more than production code.

Quick update: Chad Bradley informed me that it is possible to test private methods using Groovy. I would be hesitant to use the technique without knowing for certain if it’s a bug that will get fixed or an actual language feature.