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
Deque. Here’s the code we had:
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
Deque and, in particular,
LinkedList as the implementation – a nice flexible data structure.
The build on this project takes about 15 minutes.
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
LinkedList might behave differently.
Then I noticed the use of the
peek– that must be right
Why are we treating a stack as
pop? Surely it should be
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
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.
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.
Opinions expressed by Java Code Geeks contributors are their own.