When I started my first job I struggled to write good pull requests(PRs). At first, I had no idea what to put in a PR or why you needed to write one. I thought you just submitted your code as a PR and then the person reviewing it would do the rest. It seemed straight forward.
Boy was I wrong, because whenever I’d submit a PR it would explode with comments (a lot of comments). This would lead to refactoring, which led to more comments, leading to more refactoring and so on and so forth. Until a week later and it is finally done, and I’m left feeling like this:
It was not a feeling of relief. Once the PR is closed and merged, I’d feel angry. Angry for wasting peoples time, angry that it got to this position and angry that I let it happen more than once.
The good news is, my PRs have gotten a lot better. I’m now getting comments in them above the level of a linter. In fact, I now look forward to getting other developers to look at my code, and I think they enjoy reading my PRs (well as much as one can).
This post is going to help improve how you write and manage your PRs, you are going to get so good that they spark joy for those reading them. However, you may be asking why you want to spark joy in your PR?
Why you should improve your PRs so they spark joy
Improving code reviews not only helps you, it helps others to help you. Here are the main benefits you get from writing better PRs:
- You’ll show that you value your team members time: Time is a non-renewable resource. Knowing that, do you want to eat away at your team members time by treating them like your personal linter? You should be valuing your reviewers time by making your PRs a pleasure and not serving them up a big dish of spaghetti code.
- Less time refactoring more time building: The less rounds of feedback the more time you have for picking up tickets, and therefore building on your reputation and abilities. Enhanced reputation and production often leads to tangible benefits like more money (who doesn’t love money).
- Enhances your learning: If reviewers are spending less time as your personal QA, then they will be able to pay more attention to areas of growth for you. Instead of pointing out sloppy errors, they will be able to point out ways for you to write better code.
You now know the why, let’s look at the hard part, the how.
How to write better pull requests
Have a checklist
Early on you are going to make mistakes in your PRs. Once or twice is okay, more than that and you’re being rude.
If your colleagues keep pointing out the same mistakes on your PRs, like leaving console.logs in your code, or not following agreed naming conventions, add those to a checklist. You then run through the checklist each time before you submit your PR.
Below is my checklist as it stands now. This is a living breathing checklist that I continually update.
PR Checklist
- Console.logs
- Naming conventions
- Unused comments
- Unused CSS
- Design patterns match rest of codebase
- No if else
It's inescapable that you will submit a PR with sloppy errors in it. Don’t fret, just add that mistake to your checklist. Also pay close attention to any pattern of errors you are making and add them to the list. The key is to not let errors happen frequently, because it says to your reviewer that you don’t value their time.
I believe it is a good idea to add the next few techniques to your checklist, but let’s review them before we do that.
Before you submit, walk away and come back fresh
Before I submit any PR I step away from the code, at least for a few hours, ideally I sleep on it.
When you come back the next day, your code is going to look very different. You might even wonder who wrote that garbage (it was you!). That’s okay, you want this. If you struggle to understand it the next day, imagine how your reviewer would have felt.
Now is your chance to fix those silly mistakes and to try and make it more readable.
Protip: When reviewing your code pretend you are someone else in the team. Use the tools they would use to review your code like the diff checker and try to check the things they frequently call out in your code. For instance, one of my team members is a stickler for wording in test it blocks, so when I pretend to be them I go through and check that first. It’s amazing what I pick up.
Write code that explains itself
Your code should be written so that reviews don’t have to ask you questions like “what is this function doing?”.
Even if you are able to explain what the function is doing once someone asks in the PR, what happens when someone needs to understand it without the context of the PR?
Therefore, if your code is hard to follow it needs to be rewritten. At the very least it needs to have comments explaining it.
If you’re not sure how to write the function in a readable manner or you can’t explain it succinctly using comments, it’s okay to comment in your PR saying just that. PRs are a place to generate discussions, the goal of the PR is to generate the best outcome for all parties involved.
If you are going to do this just make sure you explain what you tried and why you went with this approach in the end. Make it easier for the reviewers to give you better direction and then implement that.
Have a changelist
Changelist are a simple way for you to communicate to the reviewer what you’ve changed and to give them any background context they need.
Try to think about the future when writing a change list. Imagine you’re someone 2 years from now who has to update that part of the codebase. That future reader should be able to understand the context of the change.
My mentor says “a good change list is one that explains the what and the why”. The what, is what this change achieves and the why, is why you are making this change. It should then list what specifically was changed.
You can also vastly improve your changelist by using screenshots and videos. After all a picture is worth a thousand words.
Save your reviewers time by showing them before and after images of any visual changes, and show them what it looks like at different screen sizes. Use video to give them a demo of the new behaviour in the application. Help them to help you.
Finally include any relevant links in the change list, below are some that are useful.
- Link to design files
- Link to any discussions outside of the card for example a slack discussion with a designer agreeing to any changes
- Link to the ticket
- Link to any relevant ADR’s
Keep it in scope
Scope creep is the mortal enemy of PRs.
I’m talking about those small UI issues you notice, that you think I can quickly fix that up. Well those little UI changes have now muddled up the review and your reviewer has to spend energy thinking about what they are supposed to be reviewing.
I know it’s tempting to make that quick change, but that change alters the purpose of the PR. You are better off making a new ticket that fixes the UI issue, so your reviewer can review the code they are supposed to be reviewing in isolation.
Scope creep is tricky, it is hard to resist making that small little change. But, your reviewer will thank you when it comes time to review your PR because they don’t have to switch context while reviewing your code.
How to handle a PR after submission
Even well written PRs don’t end after you hit submit. They are still being written until you hit that merge button. Therefore, to write a good PR you need to handle your business until you’ve hit merge and closed the PR.
Check your ego
Don’t take feedback personally. Your code isn’t you. You need to be the bigger person when receiving feedback. Don’t get mad or take it as a personal attack (even if it is one). At the end of the day you are in control of one thing, yourself and how you respond. You can’t control how others will behave in your PR.
If feedback isn’t clear, check the ego and request clarification in a nice way. Try something like, “Good point and I think I understand it. you please just clarify your point on X”. At the end of the day, the other person is just trying to improve your code (some just go about it better than others…developers!).
Often feedback is a gift. It can be a real positive when your reviewer spots obscure issues in your code. Why? Because it shows you are writing your PRs at a high level. When your reviewer can go past obvious issues, they get to spend their time focusing on real issues like logic, design and edge cases, which results in feedback you actually want.
Ensure that you respond to any learnings your reviewer has been kind enough to give you with gratefulness. If they have taken the time to teach you something, ensure that you respond in a way that shows your gratitude.
Refactor quickly
Drake has a line in the song One Dance - “as soon as you see the text, reply me”. I like to switch it up and sing it like this to myself “as soon as you see the feedback, refactor me”.
The best way to show your reviewer that you value their time is to be efficient in your response time. You do this by actioning their feedback quickly.
Why is this important? It is important because it reduces your reviewers need to refamilarize themselves with your code.
Look at it from their shoes. If you were to review their PR, provide feedback and then not hear from them until a few days later, it is likely you might have forgotten what the code they wrote does and why you left the feedback. So you essentially have to double review the code.
By responding quickly you reduce the need for your reviewer to have to go over the code again, meaning they just focus on the code you have refactored.
Now there is one thing to bear in mind with this, sometimes you will need to leave enough time for others to review the code. You don’t want to be constantly refactoring if there are disagreements on the feedback.
The way I approach it is to ensure at least two reviewers leave feedback before I make any changes to the code. As soon as I have at least two devs who have reviewed the code, I will quickly make the changes I need to keep the PR moving forward.
This is a judgement call and it depends on how your team works. I’d say a good rule of thumb is anymore than 24hrs between the feedback and refactor, you are making life harder for your reviewer.
Communicate changes
As I’ve read more good PRs, I’ve noticed a superior pattern of communication when changes are made. That pattern is to communicate explicitly once changes have been made, where they have been made and that they are ready for another round of reviews.
This is superior to the ambiguity of pushing changes and expecting your reviewer to jump back in because you remove a lot of uncertainty. The reviewer doesn’t have to guess if your changes are done and if they are ready for the next round of reviews. Thus, you are valuing the reviewers time. Helping them avoid re-reviewing half finished code.
It also saves the reviewers time as they don’t have to search for where the changes have been made, they can quickly click your link and go straight to reviewing that section of code.
It also helps to ensure you mark the feedback as resolved once it is. That way everyone knows that there is no need to look at this issue any further, meaning the reviewer can approve the code and you can merge it.
Finally ensure you are communicating that you have made changes and that the PR is ready for another round of reviews in a public channel.
Don’t rub it in
You will learn a lot from your senior colleagues. At the same time you need to understand that they can be wrong too, just like you will write code that is wrong and needs to be corrected. In these situations you will need to develop judgement, understanding, and persuasive skills to navigate them expertly.
It’s human to want to react to reviewer mistakes, especially if you’ve been taken beatings in your PRs. I’ve felt the frustration, that feeling like you’re being targeted. But, don’t take that mistake as a personal slight.
The important thing here is that you take the high road (as in don’t be a dick). In fact, it’s a good chance to reflect on your code again. If the reviewer is confused, perhaps your code isn’t clear enough. Take the opportunity to improve your code, or leave comments that give clarity.
Conclusion
Writing good PRs is hard, especially when you are just starting out. As you get ready to write your next PR, remember what you control, there is no need to rush from finishing the work to submitting the PR.
Start by looking over the code with fresh eyes, go over your checklist and handle any sloppy mistakes before you waste your reviewers time. Remove anything that isn’t in scope and set it aside for later. Next, check that your code explains itself, if it doesn’t address it in the PR. Finally, write a stellar changelist that shows the what and the why of the changes.
Your job isn’t done once it is submitted, you need to address any feedback in a timely manner, and then communicate it explicitly. If you follow the above you are well on your way to writing better PRs that spark joy in your reviewers.
Want more content? Follow me on twitter and get things like 5 ways to level up your developer portfolio.
Top comments (8)
When I got my job the first task I was given seemed herculean. I just didn't understand the codebase as well as I thought and I was worried that I'd make a bad impression. I am used to being able to just get into the code and solve the issue, but this time, my supervisor had a bunch of comments. It was hard not to take personally, but they explained that it's only week one, and I'm still learning. I should take it easy and just learn the code base.
That changed my perspective, for the first month there I spent majority of it being as eager to learn as I could. All the feedback I got was to me, just a way to make me a more efficient developer. This is really what made me comfortable working there.
I could also add, that if you feel that feedback is getting to you, take a beat and ask yourself why they suggested what they did. Chances are it is because you missed something, and that's ok. If you can't figure out why, then just ask them to explain they suggested you do X instead of Y. Offers a lot of clarity when you get their perspective.
Good article, thanks
Sometimes I'll open up the PR initially only to myself, and review it as if it weren't mine (or at least attempt to). I try to be open minded with it, and usually do fine some things that I'd missed when writing the code.
It can also help having your companies flavour of coding standards set up as linting rules. If you really want to go further than that, tools like SonarQube can be used to highlight more in-depth issues, like code repetition.
All of this can be done before being seen by one of your peers, which will mean a better quality of feedback that gets given on PRs.
From a reviewers perspective, one thing I would say is that it's important to leave positive feedback on pull requests, not just negative change requests. We can also change the way we frame our suggestions to change the tone of voice to something more positive and give more explanation. So instead of saying something like "don't use x, use y like we do everywhere else", we could say "have you looked at abc method? it uses y because we found it more readable than x".
So I followed this post. Write up a list of checks before I pushed my PR. Commented on the PR explaining choices I made.
And the results were staggering. My supervisor loved the PR. Constantly mentioned how it was really well done. I think this makes me think about my code a lot more in depth before making a PR. I spent a good 3 hours optimizing a query because I knew it could be better. It still wasn't the best, but it was certainly so much better.
This. This was my win this week. Finding this article has made me a very happy programmer.
Hey Patrick that's awesome to hear, it's one of the main reasons I write. Keep it up, eventually all these steps become automatic.
I agree fully with these two, good points:
And I have a third:
Thanks for sharing insights on this topic
Good points to consider especially writing clear comments and tracing steps taken with references and resources.