CodeSOD: Counting Answers

Lacy's co-worker needed to collect groups of strings into "clusters"- literally arrays of arrays of strings. Of great importance was knowing how many groups were in each cluster.

Making this more complicated, there was an array list of clusters. Obviously, there's a code smell just in this data organization- ArrayList<ArrayList<String[]>> is not a healthy sounding type name. There's probably a better way to express that.

That said, the data structure is pretty easy to understand: I have an outer ArrayList. Each item in the ArrayList is another ArrayList (called a cluster), and each one of those contains an array of strings (called an answer, in their application domain).

So, here's the question: for a set of clusters, what is the largest number of answers contained in any single cluster?

There's an obvious solution to this- it's a pretty basic max problem. There's an obviously wrong solution. Guess which one Lacy found in the codebase?

int counter = 0;int max_cluster_size = 0;for (ArrayList<String[]> cluster : clusters) { for (String[] item : cluster) { counter++; } if (counter > max_cluster_size) { max_cluster_size = counter; } counter = 0;}

Now, on one hand, this is simple- replace the inner for loop with a if (cluster.size() > max_cluster_size) max_cluster_size = cluster.size(). And I'm sure there's some even more readable Java streams way to do this.

And with that in mind, this is barely a WTF, but what I find interesting here is that we can infer what actually happened. Because here's what I think: once upon a time, someone misunderstood the requirements (maybe the developer, maybe the person writing the requirements). At one time, they wanted to base this off of how many strings were in the answer. Something like:

for (String[] item : cluster) { counter += item.length;}

This was wrong, and so someone decided to make the minimal change to fix the code: they turned the in-place addition into an increment. Minimal changes, and it certainly works. But it lacks any sense of the context or purpose of the code. It's a completely understandable change, but it's also hinting at so many other bad choices, lurking just off camera, that we can't see.

[Advertisement] Keep the plebs out of prod. Restrict NuGet feed privileges with ProGet. Learn more.