How to Correctly Review Pull Requests

Sounds easy, right? Reviewing pull requests can and should be easy… but are they being reviewed properly?

Time and time again, engineers raise pull requests followed by a peer’s “approval”. In some instances, no pair-programming has taken place, the pull requests are relatively large, and there are no supporting unit tests.

Does this sound familiar to you? If it does, read on, and please share your thoughts in the comments!

Why does this happen?

To understand how to review a pull request properly, we need to know why pull requests are not reviewed properly in the first place. There are many reasons; however, the most common relate to time constraints and/or pressure from management.

Therefore, let’s remind ourselves that we are hired as professionals to write good quality code. Taking the doctors analogy from Uncle Bob’s Clean Code:

To drive this point home, what if you were a doctor and had a patient who demanded that you stop all the silly hand-washing in preparation for surgery because it was taking too much time? Clearly the patient is the boss; and yet the doctor should absolutely refuse to comply. Why? Because the doctor knows more than the patient about the risks of disease and infection. It would be unprofessional (never mind criminal) for the doctor to comply with the patient.

So too is it unprofessional for programmers to bend to the will of managers who don’t understand the risks of making messes.

In a nutshell, we have a duty to ensure we review pull requests properly.

Why do we submit pull requests?

Why do we submit pull requests? Do we ever really think about the why?

There are many great reasons, but to put it simply, we raise pull requests for our code to be evaluated for correctness and to ensure quality. Pull requests also have the added benefit of helping new and existing developers learn the ever-growing code base as well as skilling up on new tools and technologies.

Great — so if we review pull requests properly then we get access to all of this good stuff!

So how do we review pull requests properly?

Keep requests small

There is nothing worse than having to review a large pull request.

Small pull requests focus our review and help us easily identify any issues/improvements. Also, developers can find it daunting when faced with a “wall of code”. By keeping our requests small, developers are more likely to want to review our code.

Two pairs of eyes are better than one!

Having at least two reviewers is invaluable in the pursuit of knowledge sharing. When reviewing code, reviewers can see each other's comments, enabling them to pick up on each other's knowledge.

Having more than one reviewer also helps mitigate the risk of issues being missed.

Does the code adhere to coding standards?

Coding standards help to ensure that the code is consistent. Having a single source of truth to reference helps with writing and reviewing code.

When writing code, developers tend to be more efficient as they can focus on the logic of the code rather than worrying about its style.

When reviewing, there is no debate if someone has not adhered to the coding standards. For example, if snake_case has been used for a variable name when the coding standards dictate lowerCamelCase, a comment with reference to the standards is enough.

Does the code read well?

If not how can it be improved?

Code clarity is one of the most critical and often overlooked points when it comes to reviewing code. The naming of everything within a project, including variables, classes, functions, folders, etc., should explain what the code is doing without requiring comments. Ideally, it should read like a book.

For example, consider the following variable names:

Bad: time = 4 # elapsed time in days

Good: elapsed_time_in_days = 4

The second example is semantic and can be easily understood wherever it is referenced.

For another example, consider the following function names:

Bad: function processUser(user) { ... }

Good: function addUserToMailingList(user) { ... }

We have no idea what the first function is doing; it could be processing the user in several ways. The second function is much better as it tells us what the code is doing within the function without having to dig deeper — we know it’s going to add the passed user to the mailing list.

Do not repeat yourself (keep DRY)

Ensure there is no repeated code. Repeated code is an ideal environment for bugs to thrive. If there is repeated code, extract the code to a function.

Are there errors in the logic?

Check that the logic is sound and makes sense. For example:

• Have all paths been considered?
• What happens if a variable is expected to not be null but is?
• Is the code doing what it says it should be doing?

Are there adequate tests to support the new code?

Check that unit tests have been written to support the new code. Also, inspect what the unit tests cover. Do they cover an acceptable amount of scenarios?

It’s worth noting writing tests before your code TDD (Test-driven development) is good practice.

Finally, be nice!

It’s important to be nice as well as helpful when reviewing a pull request. Try to make sure your comments are about the code and not the developer.

Be helpful by providing alternative solutions and justification as to why those solutions may be better. For example:

Bad: Why did you return the whole object? We don’t need to return all of this data from the API.

Good: The client only needs the id and name properties. Therefore, we can update this code to only return what’s needed. This also has the added benefit of reducing the payload size and reducing the risk of leaking sensitive data.

The takeaway

When you are reviewing your next pull request, use common sense, check the code properly and remember to be nice!

Originally published here.