Published: March 06, 2022 • 4 min read
Code reviews are important on many teams. Do them well, and your code ships quickly and safely. Do them poorly, and your code ships slowly and riskily. I try to contribute good code reviews. In this post, I’ll share my process.
First, I try to be nice 😊. There’s a lot of writing about how internet comments can be perceived as less friendly than the author intended, and code reviews suffer from this. The comments are visible to your team, too, so the emotional stakes can be high. Approaching the work with curiosity, saying “thank you”, and adding emojis and GIFs, are little ways to be more human.
Second, I convey my feedback in a parse-able, context-rich way via Conventional Comments. Is my comment suggesting a small but important optimization, asking a question, or raising a major issue? The labels clarify.
Third, I don’t QA. If you open a PR on my project (open source is another situation), I assume it satisfies a comprehensive acceptance criteria. Reviewers don’t have time or context to QA every PR even if they wanted to. Automated test help here because they can demonstrate what the code claims to do.
When reviewing, you have to pick a few things to care about. For me, I prioritize two virtues: readability and good design.
How understandable is the code? If it’s written in a language I know well, and I can’t figure out what’s happening in about five minutes, it’s not readable enough.
Most of the time in web, we implement predictable features. Add a form. Change CSS. The code isn’t reinventing the wheel. So, if I can’t make sense of it in a few minutes, either the code isn’t readable, or the PR is too big.
Consider this contrived example.
cookies() {
const cookies = this.snacks.find(snack => {
if (snack.name === 'COOKIES') {
return cookies
}
})
return cookies ? `${cookies.attributeA}, ${cookies.attributeB}` : null
}
This function works, but it isn’t highly readable for a variety of reasons:
cookies
is an iffy name for something that might be null
. What about the
maybe convention; could maybeCookies
work? Names are hard.cookies
shadows the function. Readability decreased.'COOKIES'
is a magic string. Probably more readable, but how many times is
it repeated in the code?Small wins, all. Here’s a hasty refactor. I added a displayName
property to
snacks
, which bleeds into the next idea:
// Public
cookieDisplay() {
return this.getCookies()?.displayName
},
// Private
getCookies() {
return this.snacks.find(({ name }) => name === COOKIES)
}
Some of these changes I’d skip. But viewing the public interfaces side-by-side, I think we can agree the function is headed in a more readable direction:
- cookies() {
- const cookies = this.snacks.find(snack => {
- if (snack.name === 'COOKIES') {
- return cookies
- }
- })
-
- return cookies ? `${cookies.attributeA}, ${cookies.attributeB}` : null
- }
+ cookieDisplay() {
+ return this.getCookies()?.displayName
+ }
How do you get good at readability? Reps. Seeing the same feature implemented again and again in your language of choice shows the difference.
Is this a good strategy? ‘Good’: low-risk, extendable, conventional, boring. Are we doing the thing right? Or are we hacking stuff together, doubling down on a bad design?
Let’s say we want to change how cookie names are presented in our app. What’s
the right way to do that? cookies
might work. But we should ask some
questions first.
Is this a persistent display concern? Perhaps we should add an attribute to the
JSON representation of snacks
. YAGNI, you say? That’s the easy argument
because it’s simpler. But many multi-codebases are littered with functions like
this, presenting the same attributes as a comma separated string again and
again. Maybe we are going to need it?
I want dumb frontends. If they want a string, they ask for it. That design
argument transcends how cookies
is implemented.
Sometimes readability prevents analyzing design. If I can’t scan each individual idea quickly, it’s hard to see the big picture. I try to approach design first because I think it matters more.
How do you get good at design? Reps. Bad designs will reveal themselves if you keep coding.
Will code reviews become more systematized? Maybe. Like manuscript editing, the pen of a master editor can turn a bad first draft into The Great Gatsby. Thank you to everyone who has helped me refine my code review process.
What are your thoughts on this? Let me know!
Get better at programming by learning with me! Join my 100+ subscribers receiving weekly ideas, creations, and curated resources from across the world of programming.