One of the solutions to code quality is peer review. Some suggest that pairing – i.e. live peer review – is the only answer, I’m more flexible.
There’s every reason to believe that someone reviewing the code, fresh, will spot something that the writer didn’t notice. However, the chances of finding a mistake, or being able to work on the code in future without a mistake, is dependent on how clean the code is to start with. Essentially, it’s easier to see what’s going on through the window, if it’s not frosted glass!
In other words, having a culture of clean, simple, code is a pre-requisite for review. Obviously a review comment can be let’s clean this up a bit, but that’s not necessarily going to end as well. If nothing else, there’s only so much we might have the heart to request changed in code that’s not that bad. Subsequent re-reviews of the changed code tend to be less fresh than the initial review.
How Bad Can It Be?
Here’s an interesting, recent case in point. I saw code which looked a bit like this:
01 02 03 04 05 06 07 08 09 10 11 12 13 14 | public List<String> getTheResults(String input) { List<String> list = new ArrayList<>(); String nextSegment = ""; while (input.length() >= 5) { nextSegment = input.substring(0, 5); input = input.substring(5); list.add("Value: " + nextSegment.substring(0, 1); list.add("Score: " + nextSegment.substring(1, 2); list.add("WouldReccommend: " + nextSegment.substring(2, 3); list.add("Initials: " + nextSegment.substring(3); } return list;} |
This algorithm works (or at least the original did, which I’ve largely fictionalised here).
The problem is that there’s a lot going on this method. I didn’t like the name list for the list, because I wanted to know what it was a list of. I didn’t like the reuse of the temp variable nextSegment. I found it tricky to see the input variable redefined in the lifetime of the function, and I found the while loop hard to parse, because I essentially had to process it in my head to see what it does.
This function is doing the whole job, operating at a couple of levels of abstraction, and that was the biggest issue for me.
I gave feedback around this, but eventually decided to suggest some rewrites.
How To Refactor This
The first rewrite was to have the code explain the fact that the input was being chunked into blocks and processed individually:
01 02 03 04 05 06 07 08 09 10 11 12 13 14 15 16 17 | public List<String> getTheResults(String input) { List<String> list = new ArrayList<>(); for (String nextSegment: breakIntoBlocks(input, 5)) { String nextSegment = input.substring(0, 5); list.add("Value: " + nextSegment.substring(0, 1); list.add("Score: " + nextSegment.substring(1, 2); list.add("WouldReccommend: " + nextSegment.substring(2, 3); list.add("Initials: " + nextSegment.substring(3); } return list;}private static List<String> breakIntoBlocks(String input, int size) { // loop over the string and use substring on it, returning a list // of the chunks} |
Ignoring the breakIntoBlocks function which is relatively trivial, look at how the above logic got a bit simpler. There’s no more redefining of things, and the loop now explains that it’s looping over the blocks.
The next refactoring was to process each block in its own function:
01 02 03 04 05 06 07 08 09 10 11 12 13 14 15 16 17 18 | public List<String> getTheResults(String input) { List<String> list = new ArrayList<>(); for (String nextSegment: breakIntoBlocks(input, 5)) { String nextSegment = input.substring(0, 5); list.addAll(processBlock(nextSegment)); } return list;}private static List<String> processBlock(String nextSegment) { List<String> result = new ArrayList<>(); list.add("Value: " + nextSegment.substring(0, 1); list.add("Score: " + nextSegment.substring(1, 2); list.add("WouldReccommend: " + nextSegment.substring(2, 3); list.add("Initials: " + nextSegment.substring(3); return result;} |
Now the parent function is operating a single level of responsibility, and the processBlock is doing the detailed work. I don’t like that it’s returning a list which then has to be immediately added to the list in the outer loop, and I could have passed a list in for it to be assigned, but I was heading in a certain direction with this change and was trying to demonstrate how we shouldn’t attack our inputs, and that functions can return things.
Quick Status Check
We’re two steps away from the end of this refactor, and it’s useful to do it in small chunks to see how each twist makes the code more easy to understand. It’s also relatively safer to refactor in this way, and we should have tests that protect us against accidental breakage.
What I’m going to show you at the end of this is a surprise that I faced during the original work, which I hope will come as a surprise to all but the most eagle eyed of reader.
The Last Turns
When I set out to attack the loop. I had a clear aim. It’s a loop which is trying to fill a list. It should surely be better explained as a streaming operation. I chose the sub optimal List returning processBlock because that’s really the equivalent of a map operation of some sorts and I knew it would help with the streaming version.
Similarly, the list.add chorus in the subroutine is a bit messy. Can we turn this into a stream? Won’t it reduce noise in the code, as well as removing a temporary list?
01 02 03 04 05 06 07 08 09 10 11 12 13 14 | public List<String> getTheResults(String input) { return breakIntoBlocks(input, 5) .stream() .flatMap(MyProcessor::processBlock) .collect(toList());}private static Stream<String> processBlock(String nextSegment) { return Stream.of( "Value: " + nextSegment.substring(0, 1), "Score: " + nextSegment.substring(1, 2), "WouldReccommend: " + nextSegment.substring(2, 3), "Initials: " + nextSegment.substring(3));} |
The above is a suddenly familiar construct. It’s just a stream which collects some data.
And wouldn’t you notice that… WouldReccommend is a typo. It’s spelled incorrectly. I couldn’t see that among the melee of implementation and boilerplate earlier on, but now it’s taking up more of the screen in a much more terse version of the function, the spelling error is jumping out at me.
Summary
It takes practice to be able to construct code like the refactored version of this, and it takes experience in seeing code in this shape to want to turn other code into this sort of shape. Code like the final version doesn’t need reading, because it’s made of familiar things. It reduces boilerplate and make errors jump off the page.
The actual behaviour of the runtime, with both first and last versions of the algorithm may be quite similar, and there’s nothing wrong with the original implementation… but it’s fundamentally harder to navigate and too much of a storm of characters all shouting for attention to review without huge concentration.
Published on Java Code Geeks with permission by Ashley Frieze, partner at our JCG program. See the original article here: Seeing Through Frosted Glass Opinions expressed by Java Code Geeks contributors are their own. |
