Category Archives: anti-pattern

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.

Advertisements

Palace Driven Development

A poor man and a rich man wanted to roast a rabbit. The poor man lit a bonfire, put the rabbit on a stick and sat by the fire holding the stick. The rich man snorted at such a simplistic rabbit roasting: What if rain comes while I’m roasting? What if somebody attacks me and steals my rabbit? I need something much more professional!Burj al Arab (Dubai, UAE)

So he decided to build a small hut to protect his bonfire. He hired some builders, and they convinced him that such a dignified man needs a real stove, not a shabby bonfire. He also needed an elegant dining room, or else he’s going to eat his roasted rabbit sitting on the sand. They agreed on the price and set on building the house.
Continue reading


each-require anti-pattern

This is going to be a little rant. This little anti-pattern I see all over in Ruby code by various people:

%w{yaml fileutils}.each { |lib| require lib }

I cannot understand why do people write it like this. This is neither shorter nor easier to read than the ‘normal’ version:

require 'yaml' 
require 'fileutils'

Continue reading