Don’t force them, teach them

Working with a large Java legacy codebase lately, I spotted a couple of anti-patterns. Following are three of most annoying and ubiquitous ones.

Numberosus Constantitis

A lot of classes define these constants at the beginning:

private static final int NUMBER_3 = 3;
private static final int NUMBER_4 = 4;
private static final int NUMBER_5 = 5;
private static final int NUMBER_6 = 6;
private static final int NUMBER_7 = 7;
/* ... snip ... */
private static final int NUMBER_17 = 17;
private static final int NUMBER_18 = 18;

which are then used like this:

PreparedStatement ps = ....
ps.setString(NUMBER_4, dto.getUser());
ps.setString(NUMBER_5, dto.getRole());
ps.setString(NUMBER_6, dto.getReason1());
ps.setString(NUMBER_7, dto.getActivityType());
ps.setString(NUMBER_8, dto.getReason2());
ps.setString(NUMBER_9, dto.getActivityType());

or like this:

List<ReportXDetailDto>[] result = new ArrayList[NUMBER_OF_LOOPS];
result[0] = getRedRisk(criteria);
result[1] = getOrangeRisk(criteria);
result[2] = getYellowRisk(criteria);
result[NUMBER_3] = getBlueRisk(criteria);
result[NUMBER_4] = getWhiteRisk(criteria);

Why? Oh why?–you say… It’s because the IT Management appointed checkstyle and checkstyle has been configured to complain about Magic Numbers.

Checkstyle skips 0, 1, and 2 by default as too frequent, that’s why only numbers from 3 up are defined as constants here.

The developers did not bother to replace the Magic Numbers with some meaningful constants, they just got rid of the warning with a couple of what should rather be called “refucktorings”.

As an added bonus to the last block of code, the NUMBER_OF_LOOPS constant is read from configuration file, so theoretically there could be other number of results, but the code here is hardcoded to provide 5 of them. I suspect that the NUMBER_OF_LOOPS was meant for something else, but since it happened to have the desired value, it has been “reused”.

StringBuilderosus Misunderstanditis

Another case is ubiquitous overuse of StringBuilder/StringBuffer:

StringBuilder sql = new StringBuilder();
sql.append("insert into xyz_object_groups values (");
sql.append("xyz_object_groups_seq.nextval, ");
sql.append(":objGrp, ");
sql.append(":desc, ");
sql.append(":object, ");
sql.append(":mainObject, ");
sql.append(":closed, ");
sql.append(":closedWeek)");
Query query = getEntityManager().createNativeQuery(sql.toString());

The developers probably heard somewhere that “you should use StringBuilder for string concatenation” and followed that advice religiously, ignoring the fact that it’s only applicable for string concatenation inside a loop.

What’s even worse, if the code was written like this:

String sql = "insert into xyz_object_groups values (" +
             "xyz_object_groups_seq.nextval, " +
             ":objGrp, " +
             ":desc, " +
             ":object, " +
             ":mainObject, " +
             ":closed, " +
             ":closedWeek)";
Query query = getEntityManager().createNativeQuery(sql);

it would not only be more readable, but also more efficient (static strings are concatenated by the compiler). There’s a lot of information on this topic on the Internet, but apparently some developers are google-impaired.

Obviousus Commentitis

The comment to code ratio in the codebase is quite high, but the comments are either large blocks of code that were commented out years ago or blindingly obvious comments like this:

synchronized (ranger) { /* Start of synchronized block */

// get form
XYZManageForm xyzManageForm = (XYZManageForm) form;

// get context
XYZViewRequestContext viewRequestContext = xyzViewActionContext.getRequestContext(request);

// set xyz for view in request context
viewRequestContext.setXyz(xyz);
// update xyz
xyz = xyzServices.updateXYZ(xyz, LogonUtil.getUserDto());

// set the xyz id to the context
context.setXyzId(xyzManageForm.getXyzId());

Note: XYZ is not an actual name.

So, the code is sprinkled generously with comments that might be useful only to someone without any knowledge of Java syntax whatsoever (but then how would they know what is a comment?) but if you check how much of the public API has javadoc, it’s less than 5%.

Conclusion

You can’t force the developers to write better code just by forcing the use of metrics, coding conventions, standards etc. You need to educate them. They need to understand the rationale behind each rule or advice and accept them. Otherwise they will game the metrics out of laziness or maliciousness and you’ll get code like above.

About these ads

3 responses to “Don’t force them, teach them

  • Ed Davies

    Not as bad as the obvious comment which is wrong: I once saw a function called PrintString with the header comment “Print a string” when actually it didn’t; it just prepared a buffer for printing but separate functions needed to be called to actually do the printing.

  • @pithyless

    It’s amazing what kind of bits are floating around in the wild in Enterprise No Man’s Land. I’ve seen far more sinister code sins, but Numberosus Constantitis is a new one.

    Thanks for the chuckle this morning!

  • Jeremy Seitz

    I love “Enterprise No Man’s Land”, it perfectly describes this kind of thing.
    Or:
    closed source == nobody will see the trash I left around

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

Follow

Get every new post delivered to your Inbox.

%d bloggers like this: