PR review practices

Sang -

I made this post to summarize several critical practices in the PR review process based on my experience. The list includes only essential rules, rather than general ones.

Feel free to pick up and adopt any of them to your team. Hope that they can be helpful.

P1. Avoid personality

In the PR review, try to avoid personality as much as possible.

The following phrases should be avoided.

  • I would like you to do this.
  • Usually, I do not do...
  • Personally, I would like this way of writing this function.
  • Based on my experience, I do not think that ...
  • Could you please change this to ...? (this phrase is sometimes acceptable if the reason is apparent).

Instead, always specify the reason for the change requests: code readability, maintainability, common practice, correctness, ...

The following phrases are rather recommended.

  • In my opinion, if you write code like this, it can improve the code readability.
  • I think that this code ... is easier to read.
  • I believe that the following is rather the common practice to be used.
  • This function is rather hard to read. Could you improve it?
  • I think this line is not correct, how about ...?

Personality sometimes is acceptable, but should be used carefully and must come with an explicit notice about it. For example:

  • I think you can write like this, but it is optional to change.
  • In the last project, I was told to use this. However, I think they are the same; it is up to you to change it or not.

Note: these recommended phrases alone are not enough for a full PR comment. Please continue reading "P3. Explicit action" to get more info.

P2. Bug report

Bugs report form should be simple and must include the following items clearly.

  • (optional) Overview.
  • Reproduction. (reproduction steps)
  • (required) Current. (current behavior)
  • (required) Expected. (expected behavior)

This rule is rather more effective if applied to the business team.
Notice the clearance of the reported items, and avoid combining them into a single sentence.

Bad examples:

  • The title of the home page is abc, I think it should be xyz.
  • After pressing the login button, the password is not cleared.

Good examples:

Reproduction: go to the home page
Current: the title is abc
Expected: the title is xyz
Reproduction: go to the home page, look at the page title
Current: abc
Expected: xyz
Reproduction: in the login page, type in a password, press login
Current: password field is not cleared
Expected: password field is cleared

P3. Explicit action

Always use phrases with a specific action, or explicit optionality to let the reviewee clearly understand what to do next.

Bad examples:

  • I think this code is better in terms of readability.
  • It is better to do this.
  • You can do x, y, z. (well, I saw this several times, and usually, I have no idea what to do next).

Good examples:

  • Could you change a to b?
  • The alternative way of writing code is ... But you can keep your current implementation.
  • It is easier to read if you refactorize the x variable. However, it is optional to make it.
  • Please do a, b, c.
  • You can make it like this. Please do it.
  • You can make it like this. But it is optional to do.

P4. Operational checking

For long discussion PRs, it is ok to push without doing an operational checking.

In general, everyone should make sure his/her code work every time it is pushed to the public place in the repository (e.g., in a PR). However, this is not required in a long PR discussion because sometimes it causes an unnecessary burden to the reviewee.

Nonetheless, the reviewer should include the intention in the PR comment.

Good examples:

  • I have pushed the fix to address the change request regarding the abc,xyz. This push does not include the operational check. Please continue the discussion in zyx.
  • I have fixed all the change requests and done the operational test.
    In a general context, the phrase "and done the operational test" can be ignored.

P5. Self-review

Do a self-review and make comments in the code.

After submitting a PR, before requesting a review, do a self-review by walking around the file changes, and making comments on the critical changes to draw the reviewer's attention.

Good examples:

  • I have added this new package to do abc.
  • I made this update but not 100% sure it works.

P6. Respect the usage of glossary

This rule is general and should be included in any communication rather than just for the PR review process.

The team should create and respect the usage of a glossary. This glossary should be as concise as possible or categorized by the level of confusion.

For example: if there are multiple types of users in the system.

  • Admin: means the user with the admin role.
  • Renter: means the user who rents the property.
  • Owner (or holder or host? The official term should be strictly pre-defined): means the user who owns the property.
  • User: a user with any role in the system.
  • Order (or transaction? Only one term should be used).
  • Login/Logout vs Sign in/Sign out: these terms are rather less confused, so they can be loosely interchangeable and categorized appropriately in the glossary.

Thank you for reading!