I recently came across these tweets from David K. Piano about code reviews:

David K. 🎹

@davidkpiano

Just a reminder that life's too short to reject someone's PR just because they did things differently than you would have done.

13:04 PM - 31 Jul 2020

David K. 🎹

@davidkpiano

✅ Does it work?
✅ Does it fulfill business logic requirements?
✅ Is it tested and do tests pass?
✅ Did the linter/formatter do its job?

If so, 🚢 it!

13:07 PM - 31 Jul 2020

David K. 🎹

@davidkpiano

"But we have a coding style guide..."

If it's not automated (e.g., via a linter or formatter), you're either wasting time by manually enforcing it, or the rules are too arbitrary.

13:09 PM - 31 Jul 2020

It reminded me of comments I used to make while reviewing pull requests a couple of years ago:

• Reorder imports by grouping them in "some-random-order-I-thought-it-was-correct"
• Don’t forget the trailing comma!
• Always use X
• Never use Y
• Avoid doing Z

Too often I was replicating “best practices” someone told me in the past or I’ve learned by reading a blog post like this one and I’ve followed them blindly without questioning why I was doing it.

However, none of those things were fixing bugs or adding value to the product. I was just making everyone’s lives a nightmare by nitpicking on small things.

Most of those comments could be easily fixed by using ESLint and Prettier. Some of them couldn’t but they were actually useless. Just because I’d do it differently, it doesn’t mean my way is better. As David Piano said, “if it's not automated (e.g., via a linter or formatter), you're either wasting time by manually enforcing it, or the rules are too arbitrary.”

If I could go back in time and give any advice to my old self, it would be: "Hey, dude. Take it easy. Chill out a bit and focus on what's important."

But what's important?

In my experience so far, code reviews are useful for three things:

1. Avoiding bugs
2. Learning
3. Teaching/mentoring

When we make a generic comment like “Never use Y”, we’re neither avoiding bugs nor using it as an opportunity for learning or teaching. Instead, we should be more specific to the current case:

I think we shouldn’t use Y here because explain your reasoning. It might cause a bug like this problem example. We could fix this by doing solution example.

What do I do now?

As a rule-of-thumb, I try to remember the following:

• Is my comment helping to prevent a bug? Then, it’s also an opportunity for teaching. So, I try to answer the following questions: What bug is it preventing? Why does it happen? How can we fix this?
• I don’t understand this code. Then, it’s an opportunity for learning: “I’m not sure I understand this part. Please, could add some comments to explain your reasoning?”
• I understand this code, it doesn’t cause any bugs but I’d use a different approach. I either leave it alone because it doesn’t change the end-result or I use it as an opportunity for both learning and teaching: “Have you considered doing describe your approach? I think it could help us add some benefits/metrics of your approach here. Thoughts?” This way, I can both teach my approach (in case the other person isn’t familiar with it yet) and learn the reasoning behind not using it (in case that person is familiar with my approach but has a reason for not using it, which I might not have seen).

Real-life example

Some time ago I came across a PR that was fetching millions of items using async/await inside a for loop:

for (let post of posts) {
  await doSomething(post.id);
}

My code review was something like this:

I think we shouldn’t use this for loop here because it’s blocking the loop. Every post has to wait for the previous request to complete before making another request. This makes the request slower (feel free to run a benchmark and correct me if there’s a mistake in my calculations). We could fix this issue by doing the following:

const batchActions = posts.map(post => doSomething(post.id));
await Promise.all(batchActions);

This way, we're performing those actions concurrently, which results in a faster response. Please, let me know if that makes sense or is there something else I’m missing here.

The intention behind my comment was:

• Avoid a performance bug that could hurt the application.
• Explain why that issue was happening (blocking the loop).
• Give an example of what we could do to fix that issue.
• Leave it open to be questioned in case my metrics were wrong or I wasn’t seeing some other problem that made them use that solution.

Takeaways

Make sure your code review comments are actionable and actually adding value to the product. Don’t waste time on nitpicking things that won’t change the end result. Does it work as expected and is it passing your CI pipeline? Then, move on.

Code reviews are meant to improve the product and speed up development. If they’re slowing you down and hurting the team, then maybe it’s time to review your practices. At the end of the day, “just chill out and enjoy the ride.” 😉

Follow me on Twitter