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.