How to review Pull Requests like a Google Engineer

I started learning git during my third year of studying my grade in computer science, and, in my opinion, it was way later than it should have been. However, in my experience working as a software developer, in most programming jobs you will need to have some good knowledge of it since you'll need it daily. The thing is that git itself is simple in its concept, but it can get hard to master. So getting new ideas to add to your overall git skill is something that you'll welcome.

I've been researching "the proper way" or "how to improve" pull requests, and I found some points that were interesting or that maybe I knew, but I didn't give them enough attention. In particular, I found this guide proposed by Google to be very helpful, and I want to share with you the most important points I got from it.

1) The mentor mentality

Code reviewing is a great opportunity to teach our colleagues technical topics, features of the programming language we are working on, project-specific business logic... and knowledge we have acquired over time as developers.

It's always nice to help others learn about what we know and also give them the opportunity to show us another point of view that we had not thought about through responses to our comments. We should always show our ideas with respect, empathy and constructive criticism.

A good mindset would be to figure out how we would like to have been treated when we started as developers or try to communicate as someone who's properly taught us in the past.

2) Technical facts over personal preferences

There will be situations where we feel the right way of doing something differs from what our partner suggested. Although sometimes it's difficult to detach ourselves from our ideas, we should always favour technical facts over opinions and personal preferences.

For example, there are many algorithms for sorting. Still, as intuitive as we may find it and as much as we like the bubble sort, in terms of computational complexity, we should always favour Quicksort or any other algorithm that scales better, so this would not be a proper reason to disagree with our colleague's implementation.

3) Moving from general to specific

The best way to review a pull request is to start from the most general aspects and close our scope until we get to the more concrete parts.

1. First, we should read the title and the description and see if the Pull Request solves the problem it is supposed to. Requirements sometimes need to be understood, and before we spend time reading very complex logical expressions, it can be of better use to first check if the code is (at least) pointing to do what it is supposed to do. If not, we should ask our partner to fix the pull request immediately.
2. Secondly, we should search for the code that implements the new functionality. This is usually (but not always) the file or files that modify the biggest number of lines of code and logic. Inspecting these changes will give us context for the rest of the code we will review. If we have difficulty locating the most important changes (because the PR is too long), we can always ask the developer directly where to look or even request him/her to split it into several smaller pull requests (taking care of not splitting functionality, which would be even more undesirable); smaller pull requests are easier to fix and review. If we find major issues such as a lack of requirements, unnecessary complexity, or improvable design..., we should ask for a correction as soon as possible since inspecting code whose main functionality might change soon isn't productive.
3. Finally, we can review the remaining code without worrying about the order shown by our code reviewer tool, already with the context of what the Pull request does. Google even recommends that at this stage, we start with the test files, as they can give us valuable information about more realistic use cases (And if you're not testing your code, you should also start doing it!).
4. Bonus point: nowadays git web platforms include great tools for conducting pull request reviews. However, as good as they might be, they can't offer the same level of in-depth analysis as having the code on your own machine. In order to give proper feedback, it's sometimes necessary to clone the code into your local machine, execute it, check for unused references in code with our local IDE, run the tests, check your own custom syntax highlighting, and so on. We encourage you to fetch and pull the code and run it to improve the quality your pull request reviews.

4) Respect your partner's time

As developers, we are expected to review and test our code as much as possible before making a pull request not to waste the time of those reviewing it. Reviewers should attempt to review and make suggestions for their assigned pull requests as soon as possible. Our colleague may retain the task's context if it takes less time.

This does NOT mean that you shouldn't take your time and carefully read the code you are reviewing or that you should prioritise the code review over your tasks. Instead, it means that it is important to have a good feedback flow where developers know that their code will be corrected in a reasonable time, as it allows them to do better planning in their tasks and, in general, to have a better programming experience.

Google states that a pull request should not be on hold for more than one working day and that if we are not in the middle of an important task or one that requires a lot of concentration, we should review it as soon as possible. If we cannot do so for the reasons described above, this should be one of the first tasks the next day. According to Google, not following these guides slows down the pace of development for the whole team, generates discomfort among developers, and, most likely, the health of the code base most likely will be affected.

5) Write down quality comments

To write a quality comment, we will have to take into account several aspects. First, we must be respectful, avoiding personal criticism as much as possible. This is because we are evaluating code, not people.

Example of a bad comment
"Why have you used threads when it is obvious that there is no benefit to concurrency?"

Example of a better comment
"Using concurrency adds some complexity with no benefit that I can see at first glance. Since there is no improvement, I think we should use single threading rather than multithreading."

In the second case, we don't refer to the developer directly; we explain the problem and make an improvement proposal. These last two steps are not always mandatory, but they are very desirable, especially when we can teach something new and not so obvious.

Your improvement proposals should guide the developer rather than give away the complete solution. Remember that the Pull request is the developer's responsibility, not yours.

This will allow our team to grow as developers through research, and it may even be the case that, motivated by your point of view, they will come up with an even better solution that you had not seen before.

Another great tool for writing higher-quality comments is using tags to help developers understand their type and priority. This way, he can easily spot which comments describe problems, which are positive feedback, and which are simply ideas. This helps distinguish what needs to be changed, what is an opinion, and what is just a thought.

If you don't know which tags to use, you can use the ones proposed by Conventional Comments. Conventional Comments are a way of commenting on pull request reviews where we use a given set of tags at the beginning of a comment to clarify the intention of our message. For example, we can use the tags "praise" for praising the reviewer, "question" for questions to the developer when we are not sure about why something is being done, and "issue" to point out a specific problem. If you want to know more about this way of commenting, you can check out this resource.

6) Give positive feedback

When we make Pull Requests, it is very easy to fall into a dynamic of looking for mistakes and pointing them out, even if it comes from a constructive mentality. But sometimes, we forget that pull requests are also a good opportunity to praise our colleague's work and what they have done well: the quality of details and good practices. This contributes to a good team atmosphere and motivation.

Examples of situations to give positive feedback may be:

• Cleaning up messy code in an algorithm that is not clearly understood.
• Adding new tests and coverage that you hadn't thought of in the first place.
• Something you have learned while doing the PR review.

A work partner of mine told me about trying to make at least one praise comment per PR review, and I feel like it's a good rule of thumb to follow. Of course, this is not mandatory, and sometimes it might not make sense, but it is a great way to make a positive feedback habit.

It is encouraged that you give more positive feedback to your colleagues because even if sometimes we have to correct code, we should not try to be examiners but mentors that help the people we share our job to grow as much as they help us to grow with their feedback.