Who approved THAT?

Everyone is doing code-reviews. Usually it is an integral part of any QA process. Of course every one knows what a code review is. At least this is usually assumed.

In my experience the opposite is the case and I did not yet find two different people having the same approach in doing code-reviews. More interesting: People are often not aware of their different approaches and consider their approach as THE correct way. This is dangerous because the code review culture in a company can easily degrade so badly, that the review process only wasting time of developers. Ensuring a good review quality is an important task the management has to feel responsible for.

I recently held a meeting with a development team about code-reviews. While at first the usefulness of the discussion was questioned, that meeting finally resulted in a very productive meta discussions about coding.

From my experience there is at least one common understanding of code-reviews that is generally shared between people:

Someone is looking at the code changes (pull request) of someone else and they are only accepted, after someone else found them “okay”.

In practice, the definition of “okay” is very different.

This is a list of possible “okay” criteria I experienced (it is not complete for sure):

• Code does not include any problematic architectural or design decisions
• Code change is complete
• Code does not contain bugs
• Code will not start to behave buggy in the future
• Code complies with coding conventions
• Code provides a good user experience
• Code can only be approved if there is no way to write it better

While I consider some of those goals as problematic, an organization should at least make sure that everyone doing (or relying) on code reviews shares the same understanding of what qualifies a pull request for being approved.

The different understandings of code-reviews can have problematic consequences.

I experienced the following ones:

• Reviewers start to enforce their own coding conventions. Usually the cause here is the lack of common coding conventions
• Reviewers are urged to require at least some change, just to prove that they did a review
• Developers start lengthy discussions about the perfection of less important parts of the code.
• The reviewer does not check the important design decisions at all but only looks out for code styling issues.

Additionally there are some more common problems originating from a bad review culture.

• Reviews are taking very long, either because of lengthy discussions or the developers having something more important to do than caring for submitted pull requests. Not caring for open pull requests will cause additional work (or bugs) for having to merge an old pull request with changed mainline code or a new tasks depending on the changes in the pull request. Additionally this results in bug fixes or improvements not being released to customers and bugs being reopened or reported again just because the fix lives in a feature branch only.
• Reviewers start discussions about things completely outside of scope of the PR. In example: Because the author added another data fetch call, the reviewer questions whether the fetching library used in the project is the right one.
• The slowness of approval processes because of their usual asynchronous nature. Ping-Pong games between reviewers can take weeks when one of them puts no priority in caring for the PR. This not only problematic because it keeps features and bug fixes away from mainline for longer than necessary. It can very well happen that you does not remember what you thought when writing your comment at the point in time the answer finally arrives.
• Productive developers are slowed down by the less productive ones because they have to wait for their approval and gather open pull requests
• After some weeks of pending approval usually some product manager will shout “I need that change NOW!”, what usually speeds up things drastically. This often means that an approval is given just because there is no time for changes anymore. An author of a PR may circumvent ANY changes to his submitted code just by being unresponsive.

 

Establishing a positive review culture.

Probably it is helpful to commonly accept this one:

Doing code-reviews is hard

Reading and understanding multiple pages of two-pane code comparison screens is heavily demanding. For me it is the part of development that is likely to cause the most headaches. It requires very good coding skills (and trains them of course). Not every developer is able to provide a quality review of quality code. There are people that will say now “Oh, a pull request have to be concise and easily readable”, but this will not happen because providing nice pull requests collides with other constraints of development. Just because doing code-review is so hard, you need to limit the expectations in what they can archive.

After having done this, you should define the goals of the code-reviews in you development teams. I listed some options above and will now add my thoughts on them.

Code does not include any problematic architectural or design decisions

This is the most important goal of code reviews and what everyone should focus on. In practice the problem is, that not-so-senior developers are often not able to understand the full scope of a pull request, especially when it originates from a more senior developer. Instead of looking at the relevant parts of a PR, those reviewers will dodge to more simple questions like code styling when requesting changes.

Code change is complete

This is important but rather difficult. You do not just have to check the changed lines of code, but also think of the lines of code that have not been touched but should have. This often requires good understanding of the code base which not every member of a team will have. It is an import goal but cannot be fulfilled by every reviewer.

Code does not contain bugs

I do not think that there are many people out there that can find ANY bug by just looking at a pull request. Reaching this goal by doing code-reviews is so unrealistic, that it will hurt your QA processes when you choose it nonetheless. When the approving reviewer takes responsibility for the correctness of the PR, this will make reviewers reluctant of giving their approval. This will result in long pending PRs with all the negative consequences I have written above.

Code will not start to behave buggy in the future

On the one hand you should of course request changes to code that will obviously break in the future. On the other hand it can be very unproductive to invest time in possible future code changes that may or may not take place.

Code complies with coding conventions

This is goal is right, if the convention is really defined by the team. What is not defined is not defined and should not be requested in a code review. Important coding conventions should be ensured by linters, not reviewers. In general, the strictness of reviewers at this point should match the time the team spends on finding conventions. This means, if the team is not taking time for finding conventions, you should not start wasting time on it in a PR.

Code provides a good user experience

The user experience is usually ensured by (UX-) designers, not coders. I have seen developers holding back approvals just because they do not like its appearance in the browser, although it behaves as shown in the specifications. I do not thing that this productive behavior. Whether a pull request provides a good user experience should be checked by the ones responsible for the UX in their own process, not as part of a code review.

Code must be perfect

Why it is perfectly okay to provide suggestions for better code, this should usually not block the merge of a pull request as the code is not problematic for concrete reasons.

Approving pull requests must have top priority for everyone

Pull requests are not last but one of the first priorities. Authors of code need their feedback directly after they have written the code, not one week later. The features and bug fixes that have been developed should go live soon and not stick in your code repository until they become dusty and conflicting with other branches.

Additionally it is important to NOT make reviewers responsible for problems resulting from a PR they approved. The blame game “Who approved THAT?” will only teach developers, that the best strategy to avoid trouble is not to approve anything. This will not help your QA at all.

I also found it efficient when author and reviewer of a pull request discuss the code in person instead of using text chats. This can speed up the discussion drastically.