Thursday, February 12, 2009

What should code comments do?

Below I've posted some code I just had to look at. I've got nothing against this code; it's a nice clean class, simple, I'm not aware of any bugs in it.

It's easy to figure out what this code does, just by looking at it. It takes a slash-delimited string ending in "war", like the one in main(), and deletes the third token if it contains only decimal digits.

But WHY? What problem does this class solve? What is Geronimo, why is the string "war" important?

I can't help but think that someone discovered the need for this code the hard way, after time spent looking at Geronimo code or documentation, talking with peers, perhaps after fixing a bug report from the field. All that information has now been lost.

Perhaps the need for this applied only to a particular version of Geronimo. Perhaps it only turns up in a peculiar use case. Perhaps the original developer's understanding was flawed and this code is never actually needed. There's no way to know, and anyone who encounters this code in the future will have to try to figure out how not to break it. Very likely, it actually does do something important but it's not covered in the test suite, and any breakage will be discovered as a regression in the field, when some user tries to update to the latest product version and their application no longer runs.

It's like a post in the middle of the living room: you figure it's probably supporting some weight above, but how do you know? So you can't remodel the room, because the second floor might collapse. But maybe the builder put it there because they were planning on a hot tub on the floor above, where now you've got a walkin closet. Now you've got to hire a structural engineer to do the same calculations again, because the original rationale has been lost.

Well-written code shouldn't need to explain what it does. But it should explain why it does it. What other options were considered? In what situations is the code necessary?


public class GeronimoLoaderNaming {

public static String adjustName(String name) {
if (name != null && name.endsWith("war")) {
String[] parts = name.split("/", -1);
if (parts.length != 4) { throw new RuntimeException("unknown format: " + name + ", # parts = " + parts.length); }

if ("war".equals(parts[3]) && parts[2].matches("^\\d+$")) {
name = name.replaceAll(parts[2], "");
}
}

return name;
}

public static void main(String args[]) {
String name = "Geronimo.default/simplesession/1164587457359/war";
System.err.println(adjustName(name));
}

}