Jake Worth

• github

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.

What I Bring

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.

What I Look For

When reviewing, you have to pick a few things to care about. For me, I prioritize two virtues: readability and good design.

Readability

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:

• It’s doing two things: finding something in a data structure and displaying it. Single responsibility this is not. And maybe that’s okay! But if it’s surrounded by many other lookups, we might have an opportunity to abstract.
• cookies is an iffy name for something that might be null. What about the maybe convention; could maybeCookies work? Names are hard.
• The variable cookies shadows the function. Readability decreased.
• 'COOKIES' is a magic string. Probably more readable, but how many times is it repeated in the code?
• I don’t like the ternary at the end because it’s busy and long. If the return must be truthy or falsy, would a double ampersand or optional chaining be better?

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.

Design

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.

Wrapping Up

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!