Best Practices for Code Review and Pull Requests. Insights from Developers

One of our clients asked how we do the code review at UPDIVISION and if we can provide some examples of best practices. This looked like an awesome opportunity to discuss with developers and collect some fresh insights.
 

If I had to sum up all the remarks received in only one phrase, I would say that code review is about teamwork. And it somehow defines team health as much as code health. Sometimes we forget that it involves two parties: the coder and the reviewer. So in order to make it work, we need a set of principles to guide both. Most of the teams have their unique tacit consent (gained with the previous experience) upon their standards, so this article is an effort to put them in writing.
 

Code reviews improve throughout a project’s sprints, as team members get to know each other. If it gives an opportunity for growing, self-improvement and solving conflicts, it’s a sign that it’s working.
 

But what are some examples of best practices and standards to be followed?
 

Let’s see what the UPDIVISION developers say:
 

For Coders:

• “The pull request should bring all updates/improvements that were specified in the task, not partially. In case there is a blocking issue, it should be discussed at that moment.” (Ovidiu)
• “Double-check and mark all subtasks within the task.” (Mada)
• “In general, for a pull request, it would be great to make sure its final version is testable and stable.” (Ovidiu) Always test your code, take pride in not leaving bugs for the testers to find
• “Coder must make sure there are enough explanatory comments in long functions/files.” (Marian)
• "Explanatory comments are great. They’re not necessary for long functions, but for complex functions that need more clarification. I think developers must try to choose self-explanatory function names if possible, if not, write a comment!" (Ahmad)
• “I always make sure to compare differences before sending a pull request, double-check if all aspects are taken into account and if there is nothing that is not needed anymore in the code.” (Ovidiu)
• “When a new feature or bug fixes are ready, it would be great to make sure this is the best code version by doing some refactoring at the end.” (Ovidiu)
• “The commits in the PR should describe exactly what happened (fix, feat, refactor etc).” (Ovidiu)
• “Coders need to make sure they merge the develop branch before submitting the PR, so we can avoid conflicts.” (Marian)
• “I think we all agree that using check boxes is useful.” (we use these when submitting pull requests too). This is nothing too fancy, but increases efficiency a lot. There can be a very simple template that works for your team and it may sound like this: 1. Tested links 2. Double checked X 3. Ran automated tests etc.
• “To find the context easier, PR names should have the same name as the task names in the project management tool we use (comma separated) associated with the name and follow the agreed naming convention: CU-{task_id} followed by a short description of the PR.” (Marian)
• “Coders should double-check the target branch.”
• “Deprecated/Unused code must be removed.” (Marian)
• “Make sure no console.logs are deployed to staging/production, unless there's a debugging session on that environment.” (Marian)
• “Fixes regarding other parts of the code can go in the same PR, but this is something that should be discussed within the team.” (Ovidiu)  

For Reviewers:

• “From my point of view and what I have experienced: If a PR is kind of blocking and the coder's next tasks depend on it, the reviewer should try to prioritize it if possible. Maybe it is better to create a PR based on features, not based on tasks, even when it makes the PR very big. Feature-based PRs are less dependent on other tasks, so less blocking. On the other hand, they can be fully tested as a feature and understandable by the reviewer as a whole, thus saving some time.” (Ahmad)
• “Reviewers should look if there are ways to simplify/improve code for easier reusability/maintenance. It may increase the time of the code review, but in the long-term, you may actually save a lot of time.” (Marian)
• “If we have multiple reviewers assigned to a PR, I think that PR should be merged only once all reviewers have approved the code.” (Ahmad)
• “Reviewers should keep a review diary or keep some notes for later conclusions.“
• “Reviewers should make sure the naming of variables/files is intuitive and easy to read, should some new developer ever need to edit/maintain the code.” (Marian)
• “When I do or I get a code review, I like to receive detailed notes/insights, not only “not ok”, “why did you do this?” without any recommendations. An ideal scenario is when the reviewer is explaining during a call or a quick chat how I can improve the logic or the code formatting. It is great, if the reviewer has enough time to spend some on identifying the weaknesses of the code.” (Ovidiu)
• “Everybody learns from it, including the reviewer. Both the coder and the reviewer should show responsibility when coding/reviewing code so we can avoid mistakes or pushing undesirable code to production. When solving a task, there are more solutions and all of them are reliable. The reviewer should not push this solution subjectively, if the coder has chosen another option.” (Ovidiu)
• “It's a good practice to run automated tests in front and back once developers make changes or fix bugs on features that have automatic tests implemented. And if any of the tests fail because of the changes, update them or fix related bugs if any. Briefly, PRs should not be merged when there are failing tests.” (Ahmad)  

For Both:

• “Both coders and reviewers should run the automated tests.”
• “Both coders and reviewers should make sure they tested project links before submitting a PR.”
• “If you have a PR that is for the frontend, for example, and that can't be tested functionality-wise - it shouldn’t be merged even though the error is not frontend related.” (Marian)
• “On complex/large pages or functionalities, coders and reviewers should also try to optimize for performance and to minimize the UI/UX impact.” (Marian)  

Questions I ask myself as a developer or reviewer, when doing code review:

• Is it easy to follow a reviewer? (as developer)
• Is there any additional information I can share with the developer, to help improve the process? (as reviewer)
• Do I understand what the code does? (for both of us)
• Does the code function as expected by the user? (for both of us)
• Is this following the definition of Done? (for both of us)
• Can this be simplified? (for both of us)  

General mindset:

• Share feedback and discuss repetitive situations.
• Improve our process continuously.
• Always look for ways to automate.
• Help and understand each other.
• It’s the team work leading to excellence and high-end products that really matter.  

What NOT to do:

• Never self-approve a PR. We need team power.
• If there are timeline or budget constraints, don’t waste more than half an hour on solving UI/UX problems. Ask for help from designers.
• This happens very rarely, but do not overtest if not needed. Leave some work to do for the tester.
• Don’t forget the global context (timeline, budget constraints, end-user, final goals, business logic behind it).