On code readability

Monday, June 23rd, 2008

It’s always been a subject for long and sterile debates. And yet, every now and then, I feel the itch and need to scratch it a little: coding style. Coding conventions. Javadoc usage. And whenever this comes up, it mostly ends up in “let’s update the eclipse/idea configuration files” or just “hum, yeah, we should look into this at some point” sort of reactions.

Setting up code conventions is all about code readability. Readability being all but subjective, these can hardly be completely enforced by software. I’d rather rely on common sense, but too many factors (like the food you had for lunch) have an effect on your common sense. The few points I’m about to mention can probably not be enforced reliably by any kind of tool, other than yourself. In general, Sun’s coding conventions work for me, except for their “indentation” notes. Well, the note about indentation is absolutely correct is the one I’m actually the most anal about (indent on 4 spaces, a tab character is to be represented by 8 spaces, thank you eclipse, and please don’t use the tab character in the first place anyway), but their notes about line length and line wrapping, are just plain out of scope. Generally long lines will be harder to read, yes, but wrapping them won’t help. Wrapped statements are just impossible to read. We’re not in the 70s anymore, and nobody ever has to actually work in an 80 columns terminal. So don’t enforce line length, but factor your code instead ! I’d almost be in favor of enforcing a rule that would say that 1 statement == 1 line.

Related to the above, try avoiding complex statements. Introduce variables, even if they’re to be used only once in a if statement. Give them a meaningful name, and all of sudden your code almost becomes like natural language.

Consider the following statement:

if (parent != null &amp;&amp; parent.getLevel()&gt;level &amp;&amp; parent.getChildren(//..blah).size()==0) {
 
 // .. do something
 
}

No, it is not *that* complex to read. Now consider the following:

final boolean hasChildren = (parent != null &amp;&amp; parent.getLevel()&gt;level &amp;&amp; parent.getChildren(//..blah).size()==0);
 
if (hasChildren) {
 
 // .. do something
 
}

It was not simpler to write, but way simpler to read. Why, you ask? Most probably because you didn’t even read what was assigned to the hasChildren variable. Because it has a meaningful name, you don’t need to know how the value is assigned - unless there’s a bug, which your unit tests surely have covered.

If your code is readable, why clutter it with *log statements*? I am not about to run a war against logging here, because I see how it can sometimes be useful, but if a 10 lines methods is cluttered with 4 lines of logging, I am going to ask questions. I don’t have very specific examples or guidelines for this, but avoiding splitting log statements in multiple lines, or wrapping them in if statements would already be a good start. Always question their need before adding them. Well tested code is likely to require less logging, because the tests will give you the confidence that your code works the way you expect it to work; the debug log statements would only be there as a reassurance for when your less tested code will eventually fail.

One last point that has buggered my for a while: why clutter your code with *javadoc* comments that had absolutely no value and just duplicates what your code already says ? A few examples to illustrate my point:

/**
 * Logger.
 */
private static Logger log = LoggerFactory.getLogger(SomeRandomClass.class);

The variable name and its type said it all. Why waste 3 lines of my precious attention ? (remember most computer people suffer from ADD)

/**
 * Gets the foo value.
 */
public String getFoo() {

The method name told me that.

/**
 * Does something valuable for your business.
 * @throws SomeException when something is wrong with the business.
 * @throws SomeOtherException
 */
public void doSomething() throws SomeException, SomeOtherException {

Succinct method description: check. First @throws tag tells me in what condition the exception is thrown: this is useful; check. Second @throws tag: useless. Of course, in reality, this will rarely happen, and I’ll be more often that not baffled by some eclipse-generated javadoc like this:

/**
 * 
 * @throws SomeException
 * @throws SomeOtherException
 */
public void doSomething() throws SomeException, SomeOtherException {

… which is utterly useless. (In case you weren’t sure, yes, the generated javadoc will still document these exceptions if you omit the @throws tags.

In short: javadoc is useful when you make it useful. In other cases, in just essentially makes the code unreadable. Class level javadoc often seems the most useful and less likely to get out of synch or irrelevant after 3 weeks.

Well, this being now written for posterity, I feel much better. Hopefully this will help my point made whenever I bitch about code style and javadoc, and, woohoo, maybe, help improve the general quality and readability of our code.

Share this: