CodeSOD: Try a Different Version

Back when I was still working for a large enterprise company, I did a lot of code reviews. This particular organization didn't have much interest in code quality, so a lot of the code I was reviewing was just... bad. Often, I wouldn't even need to read the code to see that it was bad.

In the olden times, inconsistent or unclear indentation was a great sign that the code would be bad. As IDEs started automating indentation, you lost that specific signal, but gained a new one. You can just tell code is bad when it's shaped like this:

public List<Integer> getDocSectionsChanged(CustomerVersionTag versionTag) {Set<Integer> sections = new HashSet<>();for (Map.Entry<String, List<String>> entry : getVersionChanges().get(versionTag).entrySet()) {for (F.Tuple<CustomerVersioningDocSection, Map<String, List<String>>> tuple : getDocSectionToSdSection()) {for (Map.Entry<String, List<String>> entry2 : tuple._2.entrySet()) {if (entry.getKey().startsWith(entry2.getKey())) {for (String change : entry.getValue()) {for (String lookFor : entry2.getValue()) {if (change.startsWith(lookFor)) {sections.add(getDocSectionNumber(tuple._1));}}}}}}}return sections.stream().sorted(Integer::compareTo).collect(Collectors.toList());}

Torvalds might not think much of 80 character lines, but exceedingly long lines are definitely a code smell, especially when they're mostly whitespace.

This is from a document management system. It tracks versions of documents, and a new feature was requested: finer grained reporting on which sections of the document changed between versions. That information was already stored, so all a developer needed to do was extract it into a list of section numbers.

Edda's entire team agreed that this would be a simple task, and estimated a relatively short time to build it- hours, maybe a day at the outside. Two weeks later when it was finally delivered, the project manager wanted to know how their estimate had gotten so off.

At a glance, you know the code is bad, because it's shaped badly. That level of indentation is always a quick sign that something's badly built. But then note the tested loops: a startsWith in a loop in a loop in a startsWith in a loop in a loop in a loop. The loops don't even always make sense to be nested- the outermost loops across an entrySet to get entries, but the next loop iterates across the result of getDocSectionToSdSection(), which takes no parameters- the 2nd loop isn't actually driven by anything extracted in the 1st loop. The inner-most pair of loops seem to be an attempt compare every change in two entry objects to see if there's a difference at any point.

I don't know their API, so I certainly don't know the right approach, but at a glance, it's clear that this is the wrong approach. With the nesting code structures and the deeply nested generics (types like F.Tuple<CustomerVersioningDocSection, Map<String, List<String>>> are another key sign somebody messed up), I don't have any idea what the developer was thinking or what the purpose of this code was. I don't know what they were going for, but I hope to the gods they missed.

[Advertisement] ProGet supports your applications, Docker containers, and third-party packages, allowing you to enforce quality standards across all components. Download and see how!