Now that pull requests are part and parcel of most source control systems, it’s never been easier to ensure that clients get the value for money they rightly expect.

At NML, pull requests are a must on all projects. In addition, we also expect testers and developers to review test cases. Unfortunately, there is no tool that helps with this, so unlike pull requests, we have to manually enforce the rules and expectations.

I expect the following from reviews:

• Reviews always take priority over other work
• Smaller reviews are preferred over big ones
• There must be more than one reviewer
• One cannot only comment to point out a problem but also have to say why it is a problem and suggest a possible solution
• People that commented must have an opportunity to confirm a satisfactory resolution

Priority

Reviews always take priority, as the code or test case in question is closer to being released than whatever you are working on. I am all for flow and getting into “the zone”, but the fact is that if you do not make an exception for reviews, you are holding project delivery at ransom.

Generally speaking, it is highly unlikely that the code you are working on is the most important thing, and must be completed without interruptions. If that is that case, I suggest that there is something wrong with either the team balance or the planning effort made that possible.

When you don’t prioritize reviews they heap up and cause delays. Lagging pull requests and test case reviews delay testing. Delayed testing, delays user acceptance. And on and on it goes.

Size

Small pull requests that affect fewer files get reviewed quicker than large ones. If your pull requests are more often than not large and affecting loads of files, then you may have a problem with planning your stories. You should investigate breaking up your stories to more manageable pieces.

It should be a concern to the whole team if a pull request is large. This is because every touched file represents additional code risk.

It is also much much harder to keep track of the effects code changes will have on large pull requests.

Size is not an issue on test cases, as we independently review every test case.

Multiple reviewers

I have a maxim that the people NML are probably tired of hearing:

Trust but verify

This means that I trust you have tried your best to accomplish the work you have committed to, but that trust is not enough to ensure the correct implementation, and I need to verify it.

This concept extends to reviews and because coding is hard more verification is required, ergo multiple reviewers is a must.

From the perspective of the person whose work is being reviewed, the more skilled eyes looking over your code the more likely it is that your solution will work and be of high quality. You also stand to learn more from multiple reviewers, a consequence that should never be shunned.

Solutions

I often need to remind reviewers that just pointing out something is wrong, is not good enough. We all look at problems differently and view things in a subtly different context. It is therefore likely that your comment can mean something different to the reader than what you intended.

By also explaining why something is wrong and giving a possible solution, you accomplish three things:

• Your initial identification of an issue was accurate
• You assist one or more team members to learn something from the issue
• You positively contribute to a faster resolution of the issue

Verify

As a commenter, you should care about the resolution of issues you identified. More so when you have explained your reasoning and provided a possible solution. It might be that the proposed solution did not take everything into consideration, or that the end solution is very different.

Either way, there is something to be learned from checking resolutions.

Further, you need to trust but verify! You can trust that the issue was resolved, but you certainly have to verify that claim.

Conclusion

Reviewing a code and test cases is a very important part of any SDLC. Keeping a couple of simple concepts in mind, reviewing can feel less of a chore, and more of an imperative than team members welcome.