Tuesday, 18 December 2018

Are Code Reviews Waste?

"Waste" is NOT a Dirty Word

My recent talk Agile is a Dirty Word generated a lot of interest at the conference at which it debuted and on social media since. More, in fact, than I ever thought I would get for any of my talks. One of the themes I examined in this talk was waste. This came quite early in the piece and one of the points I was making is that (unlike Agile, Kanban and various others) "Waste" is NOT a dirty word. What I mean by this is that whoever the client is (in my experience at least) and whatever stage of Agile maturity they are at, "Waste" is a concept that people understand and want to reduce.

An "Innocent" Value Stream Mapping Slide

I wanted a way at my client to convey how certain activities don't add value to the business and should therefore be considered to be waste. The slide I came up with for the client was designed to illustrate a very simple value stream map and start a conversation over which activities add value and which are waste. I reused the client slide with only a change to the title in Agile is a DirtyWord. Here is the slide as used in Belgium:


I use the word "innocent" in the title of this section because, as is usually the case, nothing on this slide was ever accidental or innocent. When I originally showed this slide to our client (it has transitions that make parts of it more obvious) I knew that "CODE REVIEW" down in the waste part would be controversial. I knew that several people at our client were obsessed with the idea that code review was an important part of their delivery process. So whilst "code review is waste" was not the title of this slide when I showed it at the client, I very definitely wanted somebody to notice it was flagged as waste and challenge it. This did indeed happen which "coincidentally" let me introduce the subject of why we think code review is waste in the course of talking about something completely different.

What is Waste?

In the rather simplistic model above, I have a notional timeline for a story moving across a wall. We have to assume that the thing that is moving across the wall is valuable. That is to say, I'm assuming that some sensible work by a product owner or similar has taken place and we are confident that when this thing gets released in to production that it will return some value to our company. Note the key point here is that when this thing gets released to production it will return some value to our company - NOT BEFORE. So the clock starts ticking from the point when the story's value statement is articulated and ends when it is live in production. During that time, anything that doesn't help the story move along the wall is waste.

What Outcome is Supported by a Code Review?

One question that we often ask our clients is "what outcome does this process support?" Sometimes you can ask this question of a particularly silly piece of risk management theatre and it can provoke anything from mystification to ultra defensive cat-bear-trap response to complete silence. All three responses, by the way, are a great sign that the process being questioned is unconnected to any real outcome and thus devoid of any value. When it comes to asking this question about code review, however, I've generally received a response along the lines of "code review ensures good code quality" or "we need to check that the code works". So assuming such a response is offered, we can't argue the thing away on the grounds that it can't be linked to a desirable outcome, since "working code" or "code quality" are both outcomes we should be looking for. So why do I argue code review is waste?

What REALLY Happens in a Code Review?

Disclaimer: I am writing this section based on my own personal experience. If anybody has worked in a situation where better things happen with respect to code review I'd love to hear how those better things can be made to happen.

I would also add that all of my comments refer to an organisation where the team is co-located or at most distributed over two sites. I am not considering an entirely distributed team.

The Process

Usually, a code review is requested by a developer when they want to merge some changes into a mainline. This results immediately in the request for code review waiting in a notional queue somewhere for somebody to act upon it. This is undeniably waste, all queuing time is waste, I don't think there is any argument on this one. When somebody "picks up" the code review all they can typically see is the changes made to the code. Depending on the diligence of the reviewer or, more likely, whatever time they perceive they can spare from their "real work", they might fire up their code editor and view the code changes in the wider context of whatever code base it lives. They then annotate the changes (using whatever tool they use) with any suggestions for improvement and update the status of the code review to something like "suggestions for changes made" or whatever the workflow tool in play dictates. At that point the review goes back on the queue of the person who requested the review (more undeniable waste), they react to the suggestions as they see fit and then the cycle repeats until the code reviewer "approves" the changes.

The Comments

Again, I repeat my disclaimer. There are two types of comments that I have seen. The first type are utterly unrelated to anything the code does at all. An example of such a comment would be "We should put opening braces on the same line as a function signature, NOT the line below", or one of my particular favourites "You have used a tab character of width 3, instead of 3 spaces, for your indent" (how did this person even spot that?) These type of comments are not only entirely devoid of value whatsoever, they are irritating, probably passive aggressive (I'm still never quite sure exactly what PA means but I think it applies here) and DEFINITELY should be caught with whatever linting tool is play.

The second type of comment I have seen would be along the lines of "you used a for loop to iterate when you could have used foreach" or "this slightly different way of doing it will be more efficient". Whilst I have slightly more sympathy with this type of comment I still don't think it is worthy of note unless there is specifically something about the implementation that is "wrong" or would be likely to cause some kind of real performance issue.

A Useful Comment

A useful comment could be something like "the tests don't cover all the edge cases" or "There should be an integration test of this thing". I have never seen this happen. Why? I think the answer is that the reviewer lacks the context of the story that was played and therefore lacks the knowledge to make any really useful comments about actual functionality.

Does Code Review Support the Suggested Outcomes?

If we assume that the types of comments that get passed around fall under the not useful categories mentioned above then I struggle to accept that code reviews do anything to support the outcomes of "good code" or "working code" and I would therefore argue that all such process is waste.

What If the Comments are Useful?

This is the tricky part of my argument. I have had some people argue that there are useful comments in the code reviews that happen at their company. As I say, I have never seen this in any review that I've been involved in but I'm prepared to accept that it can (and probably does) happen that some useful comments are sometimes added to code reviews and acted upon by the original developer. Is such a code review waste?

I believe it is still waste. Why? Firstly, the queuing time is there, whatever you do. Secondly, there are at least two context switches involved for different people to support this process. Context switches are expensive and add no value to any process. So they are waste. Thirdly, there is a better solution to the outcome of "good code" and "working code". Both of these outcomes are more easily achieved through pairing or mob programming and you get shared context and therefore better bus factor, into the bargain. Perhaps, if the outcomes actually are being legitimately supported by a code review process then I should downgrade my claim from "code review is waste" to "code review is an anti-pattern". I think that may be semantically more accurate in this case but "waste" is somehow more tangible a thing to eradicate than "anti-pattern".

Can Code Reviews be Value Adding?

I mentioned above that certain code reviews might be arguably an anti-pattern rather than straightforward waste. I'm prepared to accept that they add value if the following things are true:
  • Instead of the workflow, with all the queues mentioned above, the developer requesting the review approaches other developers until she finds somebody available to help with the review.
  • The code review is carried out together between the developer and the reviewer. All suggestions are therefore discussed and collaboratively acted upon.
  • Assuming there isn't the need to do any major extra work (perhaps after sharing the context it becomes apparent that the suggested solution just doesn't do what it is meant to do) the pair will continue until they are satisfied with the result and they will then merge the changes as part of the workflow.
According to the authors of Design Patterns, an anti-pattern has the following characteristics:
  • A commonly used process, structure, or pattern of action that despite initially appearing to be an appropriate and effective response to a problem, has more bad consequences than good ones.
  • Another solution exists that is documented, repeatable, and proven to be effective.
I think I have argued that in general code review fails in its goals and thus fulfills the first part of the definition. I strongly believe that there exists at least two better solutions - pair programming and mob programming. So it is therefore my contention that code review is at best an anti-pattern and at worst (and this is the more likely scenario in my experience) waste.


No comments:

Post a Comment