Pull requests are a part of developer’s daily work. Whenever we write code, it needs to be reviewed and then added to the existing code in such a way that it’s safe and doesn’t break anything.
So what are the main problems I observe when working with pull requests? In short, these are:
- too big pull requests,
- multiple independent features in one pull request,
- dependencies and nesting of pull requests,
- lack of complementarity,
- too wide pull requests.
I think that most of you, developers, heard of those rules. Maybe you even follow them. Or you used to but for some reason forgot to continue. I may not discover new lands with this article but at least will remind of some basics that are worth reminding.
Rule 1: Make your pull requests as small as possible
The size of a pull request in the main and the most significant issue.
There’s a simple correlation between the size of a pull request and the time it takes to integrate it into the code. The bigger it is the more time is required to integrate it.
What’s more, a big pull request is a tough nut for the code reviewer. There’s a study by Cisco that says that inspection rate does not exceed 500 lines of code an hour. What’s more, after reaching 200-400 lines, 70-90% of defects have already been found.
Big pull requests are being stored in a code repository. The longer they stay there, the further behind they are from the main branch.
In an ideal world, one pull request should apply to one single task. It’s difficult at times, especially when we take into account a big refactor that touches multiple parts of the code. But when we plan new functionalities for the sprint, there is no excuse.
How to simplify it? Start thinking about creating pull requests the moment you divide your work.
Usually, we have the features described as user stories. But a user story can contain a relatively big amount of work. In this case, let’s not blame the project manager but divide the story into tasks and even a task into subtasks.
If we think the feature is too big, there is also a different way of dividing it. It’s called feature branching.
Let’s assume you work on a feature called team management. It’s big enough to contain subtasks such as:
- add user
- remove user
- modify user
- add role
- remove role
- modify role
Instead of creating pull requests to the development branch, you can create a separate feature branch called team-management and make a pull request for each coded action. The benefit of this solution is that the reviewer would see the code of the smaller parts instead of the entire feature branch.
At the end of the day, when all pull requests are merged into a feature branch, we can merge the branch itself.
Of course, you’ll say, you can’t always have all features independent from one another. That’s true. In this case, however, first you take care of the common part, merge it and then code the independent parts accordingly.
Rule 2: Remember about the single responsibility principle
When you write a certain feature which consists of other features it is important to create a specific pull request for each of these features. Creating features one by one is significant especially when it comes to code review.
Imagine writing a certain feature consisting of other features, for example, A, B and C. After writing the feature you create a pull request out of it and it comes down to review. It turns out that feature A is done perfectly and B even better. But feature C has some things to improve with lots of comments because someone didn’t understand the requirements at all. As a result, the developer takes this particular feature C and rewrites, improves, fixes the crap.
In theory, it’s not an unusual situation, however, in practice, we get to a dangerous situation when features A and B could have already been merged or maybe even on the production already, but in this pull request they are blocked. And it happens because the developer didn’t apply the single responsibility principle.
The single responsibility principle says that every abstraction should be responsible for only one thing.
In IT it touches all the things: class, methods etc. Only then we can avoid the case when coded features are waiting for the others to be dealt with. Of course, if C does not pass the test, the entire pull request gets rejected. And we’re on the best road to be delayed.
Rule 3: Avoid pull request dependencies
In general, we don’t like dependencies, no matter if they occur in methods, codes, classes, libraries, npm packages or pull requests.
To give you an example. Let’s say we have a developer named John who likes to work according to the rule of keeping their pull requests small (well done, John!).
John creates a branch called cool-feature-1 from development branch and creates the first pull request (PR1).
Unfortunately, John has not mastered dividing his work into independent slices. So in order to complete the next features, John needs the code from PR1. In order to do this, John creates another branch from cool-feature-1. Let’s call it cool-feature-2.
What happens next?
Well, there are other features to code so John repeats the procedure and ends up with PR2, and in the end of the day, adds the third feature branch and the third nested PR to the landscape (that’s not so well done, John!).
What problems does John encounter when he asks Dave to do the code review?
Dave has PR1, PR2 and PR3 to review. To him, all pull requests should be independent so it makes no difference which one he checks first. Let’s say Dave takes PR1 for a ride. He adds some comments and recommendations and sends it back to John.
What does John do? Well, he makes the changes. But wait! They will appear only in PR1 and shall have no effect on PR2 or PR3 as they don’t synchronise whenever there’s a change.
On the other hand, if Dave gets his hands on PR2 and PR3 he’ll see the same code he already commented on. Adding dependencies to pull requests is a double-edged sword. It is very uncomfortable for a reviewer as well as for the coder himself as it requires additional synchronisation mechanisms between dependent pull requests.
The cascade brings nothing more than hiccups.
Rule 4: PR complementarity
A single pull request should be complementary, meaning should create one coherent solution. In short, it’s about avoiding the horror of having:
- JSON API,
In separate pull requests.
If it comes to the review although it shouldn’t (because at this stage we should know it makes no sense), Dave sees the feature sliced into different layers of abstraction. He misses the context completely.
In order to evaluate the code correctly, we need to have a full overview of the controllers, methods, views and tests. If there is more than one method to implement, we create a separate pull request for each method. Let’s take a form with three different fields with different validations. A complementary and small pull request would contain only one action in the controller, one field of the form and one test. Even though it’s that small, the entire context is there.
This way we avoid the situation in which John makes a typo in the method name on the view layer. Provided that he decided to split the layers into separate pull requests, Dave would not be able to catch the typo. What’s more, he would probably accept all pull requests and merge them even though the code would not work.
Rule 5: Aesthetics that matter
When our pull requests are small, independent, complementary and with a single responsibility, there is still more we can do to make them better. It’s about aesthetics of the code by which I mean:
- the width of the code,
- removal of white spaces,
- self-documenting names.
When I review the code of my colleagues, I like to use the side-by-side view. It means that I have two windows opened one next to the other. It drives me crazy if I need to scroll horizontally to read.
There’s an easy way to handle the code width and standardize it. Enough if you set linterns exactly the same for the entire company:
- When on Mac only, it’s perfect if the code width is set to 80px for the side-by-side view.
- If you use an external display you can allow even 120px. But then make sure everyone can review on an additional desktop.
Each pull request in BitBucket shows us the difference between the last merged version and the new changes. It’s super cool as you don’t have to look for what changed yourself. But BitBucket can’t interpret the change so it will highlight code for review even if it’s just an additional space or empty line.
In order to avoid them in pull requests (as they add no value to the code), you can use e.g a code editor extension code editor which removes trailing spaces on file save or code formatter such as prettier.
Self-documenting pull request
The code needs to be self-documenting. It means that the names of classes, methods, variables should leave no doubt what the code does. It sounds great in theory so let’s take a look at a few examples.
What would you expect table”possible_codes” to store or method “compute_order_distances” to do?
It turns out that table “possible_teka_codes” doesn’t store possible codes but stores a set of predefined ones. Who would have thought?
On the other hand, “compute_order_distances” which is expected to compute distances only assign a distance to an object and saves it. I don’t even want to think whether a method “calculate_order_distances” found in the same code actually calculates anything.
Whenever we name methods or variables in an ambiguous way, we force the reviewer to actually figure out what we as authors had in mind choosing this particular name. Code review is not literature classes to figure such things out.
If we choose to name each piece of the code according to its semantics (describing the role and responsibility of this piece) we make a step towards a more maintainable code that’s pleasant to read.
The rules mentioned above should become a mantra for any developer who works in a team, either writes or reviews pull requests.
In fact, even if you’re the only developer on the project, there will be a moment when someone else would take over your code.
One or more persons will review your Pull Request. Don’t make your reviewer work.
The more you make your reviewer work, the greater the risk is that your Pull Request will be rejected. – Mark Seemann
Setting up a pull request framework across all the projects makes it not only easy to deal with code changes but speeds up merging and in consequence shipping of the software.
Also, as I’m a fan of open source solutions and like contribute to them after hours, I can’t imagine contributing without those five principles in mind.
Hope this article will help you write small, independent and complementary pull requests your product would be proud of. If only it could say it at loud…
Apply to App Academy: http://bit.ly/AppAcademyOnline