This content originally appeared on DEV Community and was authored by Mike Cronin
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 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.
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
This content originally appeared on DEV Community and was authored by Mike Cronin
Mike Cronin | Sciencx (2021-07-29T13:38:06+00:00) How to Make a Good Pull Request. Retrieved from https://www.scien.cx/2021/07/29/how-to-make-a-good-pull-request/
Please log in to upload a file.
There are no updates yet.
Click the Upload button above to add an update.