visit
Sounds easy, right? Reviewing pull requests can and should be easy… but are they being reviewed properly?
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? Do we ever really think about the why?
Great — so if we review pull requests properly then we get access to all of this good stuff!
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.
If not how can it be improved?
Bad: time = 4 # elapsed time in days
Good: elapsed_time_in_days = 4
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.
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 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.
When you are reviewing your next pull request, use common sense, check the code properly and remember to be nice!