Asking people to code review your pull requests can be terrifying when you’re starting out. You don’t want to bother anyone, but your work has to be reviewed! Part of growing as a developer is asserting yourself more and putting your work out there, but there are steps you can take to make your code an easy review. And once you start being in charge of projects, having easy to review code can be the thing that saves you from missing deadlines.
Leave PR line comments yourself
If you take nothing else from this article, take this: line comments are a phenomenal way to tell people what you were thinking without having to get into the “is this comment necessary?” debate. You can leave overly mechanical comments on the PR itself that will speed up the review, without cluttering the codebase with things that can be figured out. Like this:
We have to add an "aria-labelledby" or "aria-label" to each one of these sections. Otherwise, they are seen as divs and not proper landmarks by the accessibility tree.
That would be a terrible comment to actually add to the code. It’s way too long, could be figured out by reading enough of the code, and talks about QA on the ticket. But as a PR line comment, it’s great. It answers a question your reviewer might have and explains what behavior it should lead to during testing. You should add these comments when:
- The logic might take a second to work through, so a hint would help
- It requires knowledge of best practices that reviewers might not have
- It takes information from other, non-altered files that don’t appear in the review (eg, telling the review the shape of the object that a pulled in function returns, so they don’t have to read the other file)
All these comments do is speed things up and decrease the number of questions the reviewer will have. You don't have to go crazy though, you don't want to explain everything in detail, but these could be helpful hints to your reviewers.
Related: How To Write A Good Code Comment
Keep your PRs under 200 lines*
This tip is more along the lines of “Ok we all know it, now actually do it.” There’s a joke that says if you give programers 10 lines, they’ll find 10 issues, but if you give them 1000, they’ll say, “looks good!” It’s funny because it’s soul-crushingly true. If you start to get into the several hundred line territory, or even the thousands, there’s literally no way that someone will be able to understand it without taking days to figure it out. By keeping it to 100-200 line territory, there’s a decent chance that a reviewer can isolate your work against the previous code and find critiques to give. But the real trick to keeping your PRs small, starts with the ticket.
*That total doesn’t include tests, snapshots, or generated files
Anything else goes in a new ticket
This is kind of crucial because adding in a few unrelated lines here and there is probably the most common way that scope creep hits your work. It can be tempting to refactor things as you see them, but that can cause confusion when it comes to review. Did that function NEED to be refactored for your new ticket to work, or is it just something you happened to fix while you were in the file? Remember, it won’t always be clear to your reviewer.
Instead, if you notice anything unrelated to your current ticket, you should categorize the work into a new ticket and throw it into the backlog. That keeps all your tickets tightly focused and easier to work on. And if your original ticket turns out to encompass more work than you thought, break it up into multiple tickets or multiple PRs. The bottom line is: keep your chunks of work small and only refactor functions you are directly changing anyway.
"But they don't let us do maintenance!"
I have heard, and unfortunately experienced myself, how sometimes the only way to get any sort of maintenance or refactoring work done is by slipping it into other tickets. It's not great, and I'll never advocate for it. I've also done it. Just know that by doing this you're treating the symptom and not the disease. You need to work with the product teams to convey the value of this sort of maintenance work on its own. But ultimately, you gotta do what you gotta do to keep the app running.
Write tests like user stories
The reason that tests get a soft pass on that line limit is because they should be helpful to a reviewer. If they see tests like “renders a new modal when button is clicked,” “has the ‘next’ button disabled when modal first appears,” and “doesn’t enable submission until all fields have valid entries,” it’s pretty clear what your code is supposed to be doing. In addition to giving more explanations, lots of tests will take some of the pressure off of your reviewer. They are no longer the only thing between production and your code, there are a bunch of new tests that programmatically take the guesswork out of your feature. This diffusion of responsibility will make your PRs less stressful, so people will be more likely to pick them up.
Context is king
These next two points should really live in the ticket for the work itself, but the keyword there is "should." If your team has vague tickets, you should still include this kind of information in the description of the pr. And it's a PR description, not a ticket, which means you can get much more technical if need be.
Explain exactly how to test and QA it
A good reviewer should always pull down the code, and verify that it does what you say it should. This is not full blown QA, more like a smoke test. However, there are often things that could be helpful to set the system up or specific edge cases you want to test. Make sure details like that are written down in the PR description itself. You want to make the confirmation process super fast and easy on your reviewer, so a few sentences here can make all the difference.
Acceptance Criteria is a must
If your team doesn’t have “Acceptance Criteria” on your tickets by default, I strongly recommend adding it in. This will give your reviewers concrete examples of what to look for when running your code. Ideally, those situations will also be put into tests as well. Also, if you’re doing anything on the front end, adding screenshots of the before and after will help. Especially if there are several different outcomes that a user can see. If this information would be helpful to your actual QA team, you can also put them into the comments on the ticket.
Assume no one knows what you’re doing
Weird tip, but stay with me. Whenever you do a ticket, there’s always some aspect of discovery. But, there’s no reason to assume your future reviewer also had those revelations. Sometimes, people are asked to review work in projects they aren’t that familiar with. If you had some epiphany that allowed you to complete the work, leave a sparknotes version of that as a line comment.
I know on one project I did there were two indexes that kept popping up. But, it turns out, only the first one was an index, the second was actually an op-code that got called several functions deeper. I added some named variables to remove the index assumption, but I also added a PR comment mentioning the exact function that the op code was used in, and what that function returned. I solidified the fix in code, but sped up the review with a hint.
Listen to critiques
This is probably the MOST important one of all, because no matter how good your stuff is, no one will want to review it if you’re an ass. Disagreements are fine, but always remain professional and courteous when getting feedback. You never want to become that dev who can’t take constructive criticism. When someone points out an issue or question, assume they are right, and work from there. Don’t instantly brush them off or ignore them. If you’re right, prove it with examples, references, and business needs. And if it turns out you’re wrong, that’s ok! The point of a review is to use the team to come up with the best code, not just use your code.
Remember, any review where you learn something is a good review.
Happy coding everyone,
Mike