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 had to switch all these h1s to h2s due to accessibility. Given that forms are only used inline and not as pop up modals anymore, best practice states that titles should be a sub heading. The style is still h3 anyway, so when QAing there should be no change.
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.
Limit your PRs to 100 lines* or less
This tip is more along the lines of "Ok we all know it, now actually do to 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 lines 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 unrelated 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 other 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 you 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.
To be clear though, feel free to refactor and improve anything that is relevant to your work. If the very function that you're there to add something to could use some polish, go for it! Refactoring necessary code will make your review easier to read. But if a function has nothing to do with your current task, break out that refactor work into a new ticket.
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 guess work out of your feature. This diffusion of responsibility will make your PRs less stressful, so people will be more likely to pick them up.
Explain exactly how to test and QA it
If there's a special way to set up the environment, or certain inputs that need inputting, tell people! Basically, if there were ever steps that you had to take to set things up while you were doing the work, you have to write that down for your reviewer and QA team. Something like:
"In order to test the new validations, fire up the dev environment, and fill out the form to the last section. Then, try adding a bad phone number, email or name. The 'submit' button should stay grayed out, and each bad input should have a red ring around it. Specifically, add numbers and characters like !, $, ^ to the name field. Those should fail now."
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.
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.
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
Top comments (2)
Totally disagree here - this re-enforces the bad mindset that fixing bugs and refactoring are somehow separate activities outside of the normal programming flow. They aren't. They should be part and parcel of everyday programming and should be going on constantly - you see a bug, you fix it - ASAP. If you don't work in this way, everything will fall into the 'do it later' pile - gathering dust in some dark corner.
As for pulling them out into seperate PRs - that just adds work and does not make sense. The reviewers should be competent enough to review anything thrown at them.
Too much effort these days is focused on making things 'convenient' and 'easy' - often (weirdly) at the expense of efficiency and/or performance.
Counter argument: If you find a bug, why would you wait to fix it? You're working on an unrelated ticket that might take a few days to complete. Why not alert someone to the issue in a ticket, and they can immediately start working on it? Parallel work is one of the easiest ways to speed things up and let a team run more efficiently. That exact case has already happened to me, and I think it would be weird if I was on a team, found a bug, and didn't break it out so it could be done faster by the run work team.
What jumps out at me though, is this comment you made:
That sounds unpleasant. Is that your team experience? I can't speak for you, but on my team, while we do have a "do it later pile," it's for de-prioritized work, not refactors. And refactors on our relevant software are almost always taken care of before we start new work in the first place. If you have a management team that won't allow for anything but new feature work or something, then yes, you will unfortunately have to cram things into PRs. But, hopefully, you can "Single Responsibility" your tickets and keep things quick.
Also also, I hope I'm not being misunderstood: if the files and functions that you are literally assigned to need refactoring...do it. This section is talking specifically about unrelated work that you happen to notice. Improving the quality of the code for the feature your updating doesn't fall into this category. I think I'll edit this section in the article to be clearer on that. This bit stems from my experience with someone on my team who was notorious for throwing in a ton of unrelated things into his PR. It ballooned every review, and honestly did make it harder to follow the flow.