First Smell – no style

I have previously ranted about the need to follow adequate (and widely accepted) code styles: new developers joining the team will have a less steep learning curve, automated tools will be better able to provide insights and, most importantly, trivial, avoidable bugs will be easy to spot.

And, yes, your code will look nicer and your self-respect will improve too: great software developers are also talented craftsman: it really is difficult to believe that you are able to express abstract ideas and brilliant insights in code, if your craft looks like you’ve been bashing a caveman’s club on your keyboard.

This code presented all the hallmark of the “keyboard monkey”:

• inconsistent use of spaces (sometimes before, sometimes after parentheses and around operators, or none at all);
• inconsistent use of blank lines: sometimes one, at random, sometimes two or more, then none;
• no regard for the line-length (many many lines longer than 100 columns, several longer than 200, and a 257 columns record-breaking line);
• no use of the “diamond pattern,” and some (random) uses of raw collections;
• variable names with widely variable level of congruence with their true meaning (and sometimes outright misleading);
• inconsistent use of constants and hard-coded strings – sometimes, the same hard-coded string repeated several times within a few lines of code (sometimes clearly the result of code-by-copy & paste)

and on and on…

Second Smell – untestable code

This class was implemented to serve a particular API call that, in response to a client query would serve a very complex object, with deep nesting of sub-objects, and a mix of business data and specific drawing coordinates [3]

When serving such a complex object back, one would want to test it under several different scenarios and ensure that the returned object conforms to the API specs.
I guess the good news was that there was virtually no documented API because, in fact, not only there was also no unit testing; but the class was (and still remains) untestable:

public static Map<String, Object> getSomeView(Map<String, Object> queryParams){
    List<Map<String,Object>> results = SomeSearch.find(queryParams);

As you can see, this method (that serves an untyped, completely undefined Map of Objects) is static – as is the very first method called, that will eventually and execute the query against MongoDB.

This makes it impossible to mock the call, or the database, or to construct a set of data-driven tests, which would have enabled us to test the data transformation and view generation logic in the method under several different scenarios.

It’s also worth noting that the queryParams could be pretty much anything – from a paging/sorting set of options (which they are – even though, the only way to find out is to run the actual server in debug mode, and execute the API call from a client, and then see what comes up at this end).

Because, yes, you guessed it: there was absolutely no javadoc to explain what the method does.

Third Smell – Confusing code

It took me the bast part of an hour to reverse-engineer the following method; then I passed it on to my colleagues as a “Java Puzzler” and their guess were wildly confused too (I have “obfuscated” some of the code to preserve confidentiality and avoid embarassment; but, trust me, the obfuscation does indeed makes this version less obscure than the original):

// Much as it pains me, I've left the code style (or lack thereof) untouched
// PLEASE DON'T DO THIS AT HOME
private static String getSomething(boolean isOnRoute,List<Map<String,Object>> trip,
        String primarySomething, String id){
    boolean somethingKnown = isOnRoute;
    if(!somethingKnown && trip!=null){
        for(Map<String,Object> segment: trip){
            Map<String,Object> line = (Map<String, Object>) thisLeg.get(Constants.LINE);
            if(line!=null){
                String something = (String) carrierLane.get(Constants.SOMETHING);
                LOG.debug("Line something: "+something);
                if(primarySomething.equals(something)){
                    somethingKnown = true;
                    break;
                }
            }
        }
    }
    if(somethingKnown){
        return primarySomething;
    }
    LOG.debug("Unknown something:: isOnRoute:'" + isOnRoute + "' ; primarySomething:'" +
            primarySomething + "' ; id:'" + id + "'");
    return null;
}

Taking you out of your misery, all this method does is to return the value (primarySomething) that was originally given to it, if the trip isOnRoute; if not, it will tell you that and return null – and let us gloss for a moment on the fact that it requires the caller to pass in the id only to use it in a debug statement; also, note the unnecessary use of a boolean variable (somethingKnown) only to use it in the break case, and then fall into a return statement; and so many other issues there (for example, why assign the value of a flag that indicates if the trip is “on route” to a flag that indicates whether that elusive something was found?).

In the end, we figured out that this method was entirely pointless, and we removed it entirely.
Sad as it sounds, hitting the DEL key on that pile of crap was the high point of my day.

Fourth Smell – Coding by Copy & Paste

You remember when you were a kid and you were asked to “spot the difference” between two pictures seemingly identical?

Let’s play the opposite game: can you spot the similarities:

// Again, I've left the code style (or lack thereof) untouched
// PLEASE DON'T DO THIS AT HOME
List<Map<String, Object>> originSiteEvents = null;
List<Map<String, Object>> destinationSiteEvents = null;
List<Map<String, Object>> inventoryEvents = null;
try{
    originSiteEvents = (List<Map<String, Object>>) ((Map<String, Object>) ((Map)entry.get(ModelConstants.INVENTORY)).get(ModelConstants.ORIGIN)).get(ModelConstants.EVENTS);
} catch (Exception e){
    //No origin events avaislable
}
try{
    inventoryEvents = (List<Map<String, Object>>)  ((Map)entry.get(ModelConstants.INVENTORY)).get(ModelConstants.EVENTS);
} catch (Exception e){
    //No inventory events available
}
try{
    destinationSiteEvents = (List<Map<String, Object>>) ((Map<String, Object>) ((Map)entry.get(ModelConstants.INVENTORY)).get(ModelConstants.DESTINATION)).get(ModelConstants.EVENTS);
} catch (Exception e){
    //No destination events available
}

if(events != null){
    List<Map<String, Object>> eventListForLeg = (List<Map<String, Object>>) events;
    for(int j=eventListForLeg.size()-1; j>=0; j--){
        Map<String, Object> event = eventListForLeg.get(j);
        if(event.get("category")!=null && event.get("category").equals("GPS")){
            event.put("isLastGPSEvent", true);
            break;
        }
    }
}
if(destinationSiteEvents != null){
    for(int j=destinationSiteEvents.size()-1; j>=0; j--){
        Map<String, Object> event = destinationSiteEvents.get(j);
        if(event.get("category")!=null && event.get("category").equals("GPS")){
            event.put("isLastGPSEvent", true);
            return;
        }
    }
}
if(inventoryEvents != null){
    for(int j=inventoryEvents.size()-1; j>=0; j--){
        Map<String, Object> event = inventoryEvents.get(j);
        if(event.get("category")!=null && event.get("category").equals("GPS")){
            event.put("isLastGPSEvent", true);
            return;
        }
    }
}
if(originSiteEvents != null){
    for(int j=originSiteEvents.size()-1; j>=0; j--){
        Map<String, Object> event = originSiteEvents.get(j);
        if(event.get("category")!=null && event.get("category").equals("GPS")){
            event.put("isLastGPSEvent", true);
            return;
        }
    }
}

As everyone knows (or should know), coding by copy & paste is a blatant violation of the DRY principle (“Don’t Repeat Yourself”) – not to mention, it’s pretty awful to look at; it will also make you look like a lazy chump to anyone who knows the first thing about software development.

Worth mentioning that the same identical pattern (navigating nested maps according to sequence of strings) was scattered all over the 600+ lines of code of this class (did I mention that?) – so, one could have been forgiven for coming up with an utility method like the one I hacked together in less than 10 minutes to replace the abomination above:

/**
 * Navigates the {@literal path} in the given map and tries to retrieve
 * the value as a list of objects.
 *
 * <p>This is safe to use, whether the path exists or not, and relatively safe against
 * {@link java.lang.ClassCastException} errors (to the extent that this kind of code can be)
 *
 * @param map the tree-like JSON
 * @param path the path to the desired object
 * @return the list of objects, or {@literal null} if any of the segments does not exist
 */
@SuppressWarnings("unchecked")
public static List<Map<String, ?>> tryPath(Map<String, ?> map, List<String> path) {
    List<Map<String, ?>> result = null;
    Map<String, ?> intermediateNode = map;
    String tail = path.remove(path.size() - 1);
    for (String node : path) {
        if (intermediateNode.containsKey(node)) {
            Object o =  intermediateNode.get(node);
            if (o instanceof Map) {
                intermediateNode = (Map<String, ?>) o;
            } else {
                LOG.error("Intermediate node {} cannot be converted to a Map ({})", node, o);
                return null;
            }
        } else {
            return null;
        }
    }
    if (intermediateNode.containsKey(tail)) {
        Object maybeList = intermediateNode.get(tail);
        if (maybeList instanceof List) {
            return (List<Map<String, ?>>) maybeList;
        }
    }
    return null;
}

The result is that the sequence of lookups (which, again, when first encountered looked like a riddle wrapped in an enigma) now looks something like:

List<List<String>> paths = Lists.newArrayList();
paths.add(Lists.newArrayList(Lists.asList(
                ModelConstants.INVENTORY, new String[] {
                ModelConstants.EVENTS}
)));
paths.add(Lists.newArrayList(Lists.asList(
                ModelConstants.INVENTORY, new String[] {
                ModelConstants.ORIGIN,
                ModelConstants.EVENTS}
)));
paths.add(Lists.newArrayList(Lists.asList(
                ModelConstants.INVENTORY, new String[] {
                ModelConstants.DESTINATION,
                ModelConstants.EVENTS}
)));
List<Map<String, ?>> events = null;
for (List<String> path : paths) {
    events = tryPath(entry, path);
    if (events != null) {
        break;
    }
}
if (events != null) {
    for (int j = events.size() - 1; j >= 0; --j) {
        Map<String, Object> event = (Map<String, Object>) events.get(j);
        if (event.contains("category") && Constants.GPS.equals(event.get("category"))) {
            event.put(Constants.isLastGPSEvent, true);
            return;
        }
    }
}

This is still not as clean as I’d like it to be, but mostly blame Java for lacking a facility similar to Scala’s Lists:

val paths = List(List(ModelConstants.INVENTORY, ModelConstants.EVENTS,
                 List(ModelConstants.INVENTORY, ModelConstants.ORIGIN, ModelConstants.EVENTS),
                 List(ModelConstants.INVENTORY, ModelConstants.DESTINATION,
                      ModelConstants.EVENTS))

and refactoring the (abominable) use of Map<String, Object> is an ongoing activity that will take us several sprints.

Also note the anti-pattern of using a try-catch block to filter out potentially valid code paths, but avoid writing null and key-existence checks: by factoring out the checks and the class casts (with proper type safety checks) in one place, we avoid having to choose between two bad options: scatter the same repetive, tedious checks all over the place or mask out possible error codes by casting a very wide net (by letting through potentially erroneous conditions, we will make it exceedingly difficult to find out root causes of unexpected bugs).

There’s an entire blog post to be written about “swallowing” exception and error conditions.

Conclusions

Code is still largely a craft – while a deep understanding of issues related to distributed computing; a capacity for critical reasoning and abstract thinking; and an understanding of operational issues related to scalable systems are critical these days to be a great software developer, still the mastery of the craft is as important as it used to be in Reinassance Italy.

Write well-designed, clearly structured and properly documented code – and be proud of your craft!

NOTES