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:
01 02 03 04 05 06 07 08 09 10 11 12 13 | 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 rightpop– classicadd– 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.
Published on Java Code Geeks with permission by Ashley Frieze, partner at our JCG program. See the original article here: That’s Two Hours I Won’t Get Back Opinions expressed by Java Code Geeks contributors are their own. |
Agreed. Also, even if it’s not exactly my preferred format, I always find reading uniform tidy code easier. In a team, if you have to dive in and understand/fix code written by “that guy” who writes wacky code, even if it’s reliable code, it’s just makes the context switching harder.