How?
Technique #1
Review your own code multiple times before sending it off for someone else to review
Remove any “noise”
Get rid of unused imports
Check all formatting (ideally, your codebase has linting so you don't have to worry about this)
Remove any unnecessary whitespace changes (you might think this is nitpicking but it is not - it clutters up the git history for that line of code! Don't make your git history useless!!)
Clean up all experiments you did while developing (such as making certain views crazy colors or adding crazy logging)
All TODOs and FIXMEs should have a ticket number
Code comments should be clear and well-written
Bottom line: Don’t waste your colleagues’ time by sending them sloppy code to review! Don't send your code out for review until you can't find anything else to fix/improve about it. Only then is it time to bring in a second pair of eyes. In other words, be respectful of other people's time. Be professional. (And even after you send it out, keep reviewing it!)
If you exhaust your reviewer with sloppy code, they may run out of energy before they actually get around to evaluating the "big picture" of your code changes. That would be a real tragedy.
Technique #2
Are there pieces of your code that you expect your reviewer to stumble over or have questions about? If so, add a comment to your own PR before sending it to them - or consider adding code comments. In other words, don't wait for the reviewer to stumble over the code - just go ahead and explain. This saves time by eliminating at least 1 back-and-forth between you and the reviewer. Then, instead of the reviewer beginning the review by being confused, they can start off by providing valuable feedback for that bit of wonky code.
Technique #3
Separate unrelated refactors (and even related refactors!) into separate PRs. I think most of us can agree that smaller PRs are easier to review. Make your PR smaller by separating out any refactors (but link to them in the PR description! You still want to give your reviewer the whole picture).
Technique #4
If there is no way to avoid a huge PR, one way to potentially help your reviewer is to write really great commit messages and set up the commit history so that the reviewer can review commit by commit. You don't necessarily need to do this while you are developing - you can develop using your normal approach and then go back later and re-organize the commit history using git rebase --interactive. (This will be much easier if you make lots of small commits as you go, rather than a few large commits. It is much easier to re-order and squash small commits than it is to pull apart large commits.)
Technique #5
Utilize the PR description. I see far too many empty PR descriptions, where just one sentence giving some context would make a world of difference. Here are some things you might want to include in a PR description:
Screenshots. A screenshot is worth 1000 words. An annotated screenshot is worth 1 million words. This is a quick and easy way to tell the reviewer what they should be looking for. Never use words where a screenshot will do. ("Before" and "After" screenshots are very helpful in many situations!)
Mention what bits of code you want extra scrutiny on.
Include QA instructions, your reviewer will thank you and even more importantly, you will thank yourself later when a bug pops up and you need to remember how to test this bit of code
especially when you need some special combination of flags turned on/off to re-create the scenario to test
or when you need a special user account to test the code
or a special test payment method
or anything like that!
include all of that information in the PR! Don't make your reviewer spend time searching for all of these bits that you have already spent time gathering! That is a complete waste of time. (And as an engineer, wasted time should horrify you.)
If the acceptance criteria was confusing, clarify it for your reviewer, don’t make them duplicate your efforts to de-mystify it.
Why?
People aren't always at their best - you might end up with a reviewer who is under time pressure or distracted or sleep-deprived, for whatever reason. These days, you might end up with a reviewer who is trying to juggle work and childcare at the same time. We are all human - ignoring that fact would be silly. Set your reviewer up to give their best possible review by removing whatever distractions you can (ie sloppy code, irrelevant changes) and minimizing the effort they need to put in to the review.
But, why do I care how thoroughly they review my PR?
1. You don't want any of your bugs or mistakes getting merged into the codebase! This will end up with you fixing your mistake while 5 other developers are breathing down your neck because your mistake is blocking them. I don't know about you, but this is not when I produce my best work. So let's try and minimize these situations.
2. You DEFINITELY don't want any of your bugs getting into production. If you thought fixing a bug with 5 developers breathing down your neck was stressful, try fixing a bug that's affecting hundreds or thousands of users. Not fun.
3. You don't want to merge crap into the codebase. Because that's how you end up with a horrible code base that nobody wants to work in. And that's how your team loses good developers. Sub-par code bases are not built overnight. They are former high-quality code bases that simply fell victim to death by a thousand paper cuts. Mostly through sloppy, lazy, or half-hearted code review.
4. You can pretty much guarantee that as soon as you merge your code, someone else is going to be copying and pasting it. So don't merge in code that you don't want to see copied all over the codebase. (Bad code, especially, tends to multiply like rabbits.)
But, doesn't this take a lot of time?
Sure, but I think it saves a lot more time in the long run. Cleaner (ie more readable), higher quality code (ie fewer bugs) means higher velocity. Additionally, finding problems early on means fixing them on your branch, before they are blocking other devs. That also increases velocity.
Comments