Git has become a de facto standard for source code versioning and to get the most out of it, we need to understand not just how it works, but also how to utilize its most important collaboration feature: pull requests!
Most developers have heard of and opened pull requests. For many of us, it represents a huge step forward in our code delivery process. We have written some code, we are happy with the result, and we want to say that we are done. But to achieve that, we need to get our pull request approved or maybe approve someone else’s.
In this blog post, we will dive into tips and guidelines for pull request participants to get the most out of this Git feature and the process surrounding it. At Comtrade Digital Services, we always strive for the best possible code quality and this is a small piece of the puzzle that helps us achieve that goal. You might be using a source code hosting service like GitHub, BitBucket, or GitLab, but the review process is the same and the following tips can be applied anywhere.
Where is the pull request in the developer's workflow?
You were given a task. “We need this feature developed urgently,” they said. “If it were done yesterday, it would have been too late!”
This is what developers do most of the time. They listen to their customers about the features they need and then develop the code that does that, usually under some time pressure. They deliver the code and move on to the next task. But somewhere in between, they need to collaborate with other developers. There are internal coding standards that need to be followed, they have to keep the code clean, and they need to make sure they leave the code better than it was before they started working on it. To achieve this, they need to ask fellow developers to review their code, which can be achieved by opening a pull request.
First, let us look at some tips and the important questions pull request creators need to ask themselves:
- Would you approve the pull request if someone else opened it?
- Have you checked each line of code in the pull request? Is change x really needed?
- Have you made sure the code is compliant with internal standards (formatting, etc.)?
- Have you checked the code for typos?
- Does the full build pass?
- Have testing standards been met (unit, API tests)?
- Are my commits linked to the story so that reviewers can understand the business case?
The other party involved is the pull request reviewer. Reviewers need to approve the pull request and vouch with their approval that the code is compliant with project policies and does what it is supposed to do. The reviewer usually needs to answer the following questions:
- Is the code clean and without unneeded changes (empty lines, unused imports)?
- Does the code do what the story says it should do?
- Is the code properly tested?
- Is the code compliant with coding standards?
- Do changes need additional discussion, e.g., input from an architect?
- Can I suggest some objective improvements or give subjective observations to improve the coding skills of everyone involved in the pull request review and improve the code?
- Have I so far established enough trust in the pull request creator or do I need to check out the code and fully test it to approve the pull request?
Before analyzing all these bullet points, we need to understand that a pull request review is a process in which different parties need to come to a basic agreement. That means we all need to collaborate and find an acceptable solution to the problem even if we do not entirely agree with it. To achieve that, we need to focus on constructive dialogue and understand that everyone involved has the best intentions. Too often this idea is forgotten, discarded, or purposely ignored.
Guidelines for creators1. Look for acceptable code quality
The hardest thing to do when writing code is trying to evaluate the code you write objectively. It is easy to see mistakes in other people's work and hard to see our own bad practices. So how do you achieve a certain level of code quality when it seems that the mere definition of the code quality is subjective at least at the beginning of our developer careers?
There is just one answer to this question and it is pretty simple: experience. If you are a junior developer, consult with your mentor and ask for an internal code review even before opening a pull request. For experienced developers, I have just one tip: look through your code and remember previous pull requests. Apply the recommendations that you most frequently receive or give and that process should produce good code. Thinking about code changes in this way will inevitably make you think about the changes as if someone else made them.
2. Clean up unneeded changes
When asking others to review your code, you need to understand one thing: reviewers are willing to give up some of their time to review your changes. In exchange for that, you actually need to deliver those changes. No more and no less. You should show your respect for their time by checking the code twice or even three times.
When you are developing code, you might modify some lines of code or even some files, but then you change your mind and revert those changes. It often happens that developers “forget” some newly added empty lines, whitespace, etc. Please, do not commit those changes. Please, do not waste the reviewer's time, and please, do not litter commit history. Understandably, there is time pressure, but taking these shortcuts means reviews taking a detour and future maintainers of the code also taking a detour.
3. Set up your IDE properly
The next tip is about setting up your IDE properly. Set your formatter, set your import order, set your save actions, set your general project policies. Update them when they change. Countless times developers are swapping tabs for whitespaces and vice versa, reordering import orders and just generally reformatting code without any added value. Fixing your IDE settings is a one-time job, and there should be no excuse not to set it properly to comply with internal coding standards.
4. Reread the code
Before opening a pull request, carefully read your code changes line by line, letter by letter. Check for typos and proper capitalization. You will fix typos and, more importantly, you will read the code properly in the same way as reviewers will, so you have a chance to rethink your naming, ordering, etc. Have you selected the best method and variable names or would something benefit from a better name? Are private methods where they are supposed to be?
5. Make sure you are not too fast with the changes
You are not a firefighter. I have often opened a pull request where the build did not pass. Sometimes you do not run all the tests. Maybe you temporarily disable some quality checks. It happens and you learn. Just make sure that before you press that “push” your full build passes. A failing build usually happens when you address pull request comments and make some minor changes so you do not expect a failed test. Well, do not be surprised. It happens to everyone. Yes, a production hotfix is urgent, but not so urgent that you should skip the full local build or make poor code changes.
6. Cover your code with tests
What is the percentage of code on your current project which should be covered with unit tests? Do you know? If the number is undefined, you might want to talk to your dev manager.
What is important to understand about writing tests is that they are not intended for you. You coded your feature and you know how it works. You tested it and your tests will not be failing. Tests are there for future work on your code. A new developer will come in, change some code, and tests will start failing. If you did your job right, he will be able to determine quickly whether the change broke the test or introduced some new and unexpected bug. Make sure your tests are clean, easy-to-read standalone units that test precisely what you intended. Make no assumptions about the code in your tests, keep the tests small, and test one thing per test. Nobody wants to go through a huge number of dependencies to figure out what some test is testing or why it is failing.
7. Do what is described in the related story
Make sure that once a reviewer looks at your code, he knows exactly the problem you are trying to solve. The commit message should link to the related story and the story description should be accurate. I cannot count the number of times I have opened a story with the text “fix bug x”. You might want to say that someone else created that title, but you need to understand that at the end of the day, you are the one writing code. You own it. If only you know what needs to be done, then no one will be able to verify that your code works as intended. And if no one can verify your work, no one will know whether you are a good developer, not even your superior.
Reviewer's guidelines1. Determine the general effort of the author
Quickly checking the changes gives you an overall impression of the pull request you are about to review. You can see the author was in a hurry if the code is littered with unneeded changes, etc. This helps you determine what kind of code you are looking at. Is it something that you can review and approve quickly or will the code changes need a more in-depth discussion with the author and therefore a scheduled timeslot for a review?
In my experiences, pull request creators that respect others follow a common set of guidelines and commit history to deliver cleaner and better code, which is reviewed faster. Of course, some authors do not fit the description, but we are working with multiple authors, so an individual should comply with the common rules, not vice versa.
2. Figure out contexts and the scope
It would be best if you determined what the code should do. Personally, I like to begin by reading the story in the related ticket and then look at the entry points into the system to see how the change is reflected on the outside. To understand that, you might need some overall knowledge of the whole module that the code is part of. This means you need to understand what you are reviewing first before deciding whether the code is good or not. Someone changed something simple in the "if" statement? You still need to know the whole module even if changes are simple. That changed statement can have massive implications and not all bugs can be solved with changes to a single line.
3. Focus on test cases - they tell the story
After making sure that the code changeset is neat and clean and understanding what the code change should be about, you can start digging into the code. You can check the changed methods and the unit tests that go with them. When properly named and written, unit tests always tell you what the input data is and how some piece of code should behave. After checking all this, you should have a good understanding of the code and the author's intentions when writing it. After this, you should finally be ready to give the author some feedback.
4. Look for code smells, technical debt, and compliance with standards
Usually, my first feedback is to comment on the obvious: typos, improperly named methods, possible errors caused by edge cases, etc. The idea here is that once the code meets the basic standards, you can start looking into caveats of the implementation. Even if the code is totally different in the end, the original author will still learn and improve the basic skills of our craft. With experienced authors, there are usually fewer such issues at this point.
5. Check whether the code is under/overdesigned
Now you can start looking at the code in the scope of the whole module. Is the code properly placed? Have the right coding patterns been used? Are edge cases properly thought out and is code generic enough or should it be less generic? Does the code affect the whole module in some unexpected ways? Can this negatively affect some other aspects of our whole ecosystem, for example, from a performance perspective? Have security standards been met? This is where the discussion with the author starts. The feature always constrains the creator's view and your job is to make that box a little bigger.
6. Make constructive comments about the code
You need to take special care of the discussion happening in the pull request review process. You need to make sure valid arguments are made and ask architects for their input if needed. If we all want to have a working environment that produces quality code, then these arguments need to be clear and understood to achieve that goal. Too many times, authors and reviewers get into pointless arguing and all parties involved start taking comments as personal attacks, not as objective feedback about the proposed code change.
7. Approve the pull request once the code is OK and do not rush to meet the deadline
I was once involved with a project with reviewers competing who will be the first to approve a pull request. While I acknowledge there are trivial code changes that do not need special attention, we should review every single code change in detail. We need to show others that we are reviewing the code in detail and that we can be trusted. Once other developers understand that, they will automatically produce better code to ensure that requests are reviewed in a timely manner. If there are deadlines for code delivery, we should improve communication and maybe use pair programming to solve any code issues. What you should not do is cave under time pressure and lower the code quality standards as a result. You only need to make an exception once and it can quickly become the standard..
It is not about you. It is about the process!
Pull request reviewing is a process that needs special attention since it is the main place of interaction surrounding our work. Everyone was a junior developer at one point, and even with Stack Overflow and Google, we need feedback from fellow senior developers to improve our coding skills. If the review process is based on arguments and merit, everyone will benefit from it. Our customers will receive a quality product, we as developers will learn from each other, and future code maintainers will do their work faster and better. We, the developers, must first understand that we are not firefighters. If we deliver code that is cutting corners, it will come back to haunt us. And if we inherit code from someone else, we will be grateful if the code quality is at an acceptable level. We will still want to rewrite other developer's code every time; the code will not need a hotfix of a hotfix of a bugfix.
You can look at your project and the established code review process and try to improve it. Look at the tone of the conversation in the comments. Make sure you are exchanging arguments and ideas so that optimal code can be delivered. You do not need to tolerate bad code, just communicate clearly why you believe something is wrong. Too many times, people start taking comments as personal attacks and if you are able to create a culture where the focus is on the code and not the people behind the code, you are going in the right direction. But as with most things, the real challenge is to maintain it. Good luck on that path!