That’s Two Hours I Won’t Get Back

As I’ve said before around the subject of linting, there’s a limited benefit of spending time modifying your code just because an automated tool told you to. Worse than that, these tools are not infallible.

For example, we’ve been regularly adding an exclusion in for a SpotBugs warning around a perfectly innocuous try-with-resources construct, which it doesn’t quite like in Java 11. Similarly, it looks like SonarQube has trouble with a particular static import. No idea why, and it wastes time to appease these tools.

The dilemma with static analysis and doing what it says is that if you DO put the time in to do what it says, it’s hard to see the benefit, but if you DON’T, then there are some potentially worse side effects:

  • Some of the code layout starts to be a matter of opinion – and opinion varies across the team
  • Some obscure problems sit in the code and nobody notices them
  • Overall quality and attention to quality goes down

It’s the second case which is the most frustrating. Thanks to some static analysis tools, I’ve fixed a single figure number of performance, security and stability bugs recently. I don’t think any of them were a guaranteed fail, but each was potentially going to waste some of our scarce compute resource, or add risk to the project.

Had I not heeded the whole body of issues, trying to get the count down as low as possible, I might not have noticed these issues.

So, it’s got to be done. It’s like dusting. If you leave it, suddenly there’s a lot to do, and things can be in a worse state than you imagine.

The Two Hours I Wish I Had Back

One of SonarQube’s suggestions is to replace the Java class Stack which Deque. Here’s the code we had:

Stack<StringBuilder> tags = new Stack<>();

void onNewElement() {
   tags.add(new StringBuilder());
}

void onNewData(String data) {
   tags.peek().append(data);
}

void onEndElement() {
   save(tags.pop());
}

I’ve simplified it a bit. It was reading XML and allowing for a nested hierarchy where you need something like a stack of elements to allow the hierarchy to be traversed.

What I thought I could do was replace Stack with Deque and, in particular, LinkedList as the implementation – a nice flexible data structure.

The build on this project takes about 15 minutes.

It failed.

I looked over all the changes I’d made for the sake of SonarQube and started making educated guesses around which might be destructive. Though from this article, it looks like it must be the Stack refactor (restacktor?) to blame, I had some other candidates, so lost some build cycles to those.

Eventually, I reverted back to Stack and some 15 minutes later, had a green build.

At this point I’d like to thank past me for writing the test automation sensitive enough to spot this problem, especially since it was a rework of a legacy codebase that originally had no useful tests.

Did You Spot The Error?

Once I had established the fix, I didn’t want to let myself get away with not knowing what was going on and leaving things alone because voodoo… oooooh!

So, I asked myself why Stack and LinkedList might behave differently.

Then I noticed the use of the Stack methods:

  • peek – that must be right
  • pop – classic
  • add – wut?

Why are we treating a stack as add/pop? Surely it should be push/pop ?

That was the fix. Lower down the implementation details, it turns out that LinkedList treats the head element as the top of the stack, but adds new elements to the tail (which is how a linked list should work). Conversely, Vector, the underlying implementation of Stack adds to the end, and also does peek and pop from the end. If you’re an array, you prefer not to shuffle elements around.

The Thieves of Time

So there were two thieves of time here:

  • Someone using the API inconsistently to achieve stacking – leading to this weird migration error
  • The damn 15 minute build

If my build had been 2 minutes, none of this would have taken so long… this test needed a lot of apparatus to run. There’s good reasons for that, but it’s still a huge overhead and it costs real time.

TL;DR

If you write dirty code, sooner or later it will catch up with you or someone else. The linting tools, painful though they can be, ultimately do a good job in reducing the baseline oddness, but they can steal your time in the process.

One comment

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out /  Change )

Google photo

You are commenting using your Google account. Log Out /  Change )

Twitter picture

You are commenting using your Twitter account. Log Out /  Change )

Facebook photo

You are commenting using your Facebook account. Log Out /  Change )

Connecting to %s