5 things to avoid in Pull Requests Reviews

Pragmatic Nerdz
Jan 21, 2021 - 3 Minutes

Pull Request (PR) review is powerful social experience where team collaborators comment on proposed changes in order to improve them.

As any social interactions, it comes with its share of frustrations and conflicts. Here are 5 things that you should stop reviewing in PR to mitigate frustrations in your team.

1. Style and formatting review

Do not waste your time reviewing whatever can be done by the machine. I've already been asked in one of my PR to add a space after { . Instead, integrate in your build process tools to detect violations in your code, so that developer can address them before submitting the PR for review:

  • Code linters to enforce the team's coding conventions. Ex: Checkstyle for Java,  Klint for Kotlin,  eslint for Javascript
  • Code coverage to enforce the team's test quality standards. Ex: Jacoco for Java Istambul for Javascript
  • Common coding flaws using tools like FindBugs or PMD

2. Big Pull Requests

I usually find big PR much harder to review, but also require more time for review. Since time is usually a scarce resources, sometimes we rubber stamp big PRs to unblock the team, or because we trust the issuer of the PR - which I think are not good reasons for approving a PR.

When a PR cannot be broken , this is usually a sign of a design issues that lead to Shotgun Surgery code smell.

PRs should respect the Single Responsibility Principle.  Write 10 small PRs, each PR addressing one thing, instead of 1 PR addressing 10 things!

3. Design review

With agility being mainstream, developers tend to go straight to code. But if the code has design flaws, your PR will be rejected and you'll have to re-do the work using a different approach. This is a huge waste of time for the team!

Receive my Stories your e-mail inbox as soon as I publish them.
Subscribe to my Blog

To prevent this issue, get initial agreement with your peers on how to implement the changes:

  • For complex changes, go through a formal a design proposal review with the team.
  • For simpler change, do an initial PR showing at high level the algorithm/approach our about to take.  Since this PR purpose is to get an agreement with your peers, it should not be merged to the master branch! 

Once you get the agreement on the how,  the PR review will just focus on the implementation aspects, based on the agreed design.

4. Personal Preferences

Avoid discussion around subject where both parties are right. A good example I've seen are discussion about using functional vs oriented object programming.

Look at the following for loops code in Kotlin:

for (i=0 ; i<array.length ; i++){
    Item = array[i];
    ...
}

for (Item item: array) {
   ...
}

array.forEach{ 
  ... 
}
  • The 3 for loops are doing the same thing.
  • The 3 for loop are syntactically correct.
  • The generated bytecode by the JVM level will be the same

Choosing one method vs the other is purely subjective and usually depend on personal preferences and style.

I personally find that requesting a developer to use one specific approach vs another is like imposing our personal preferences.  Instead, make a non-blocking suggestion, so that it's up to the developer to apply it or not.

5. Long discussion threads

Be cautious with long discussion threads on PR. If you are not able to get an agreement on on a sticky point in  your PR:

  • Add another reviewer in the thread to get another point of view. If you are in minority, comply and move on :-)
  • Go a talk straight to that reviewer.  You'll be surprised how much the tone of your voice, your smile, your body language can help to build trust, address sticky points and find a compromise with your reviewer.
    This is even more important now in COVID time where people mostly Work From Home. Start a video conference to initiate the verbal communication!
Coding
Feedback
Pull Request