I recently came across these tweets from David K. Piano about code reviews:
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:
- Avoiding bugs
- Learning
- 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.” 😉
Top comments (30)
To be honest, the example CR, you’ve mentioned is 100% correct but the other way around than the article is trying to prove. Code reviews are not there just for the sake of the product but yours as well to get a second opinion over the code also CRs are a great tool to pass down knowledge by other more senior devs. I’m always happy to get feedback on my code, where I could have wrote better, more readable and performant code.
I agree with you. I kind of feel like I'm in a rut with the team I work on because I don't think they even read the MR anymore. I miss being on a team that actually would suggest good feedback.
Good points here. I agree with you that it's important to have some perspective, and not blindly follow "advice" you read in blog articles.
I once came across a series of articles by a maintainer of an OSS project we were using. After contributing a bug-fix and while waiting for the PR to be merged and release, I started reading through some of their blog posts, and a lot of their thoughts around PRs were very much antithetical to the points you raise: a lot of comments around control, protection of the codebase, nothing really about mentorship, etc. But the thing that disqualified all of that was it took a year for them to release the next version.
I like your approach.
My best experience with code reviews can be explained with a story:
Once were building a counter for a customer product and the feature got built, but when tested had a serious bug, so the tester reported the bug and then it went in to be fixed. The developer fixed the bug, but the tester found another problem. After 2-3 days of this "feature tennis" I became curious so I went up to see what the developer had done and ONE GLANCE to the code told me that the implementation would NEVER WORK.
This has happened a few times in my career. Sometimes junior programmers (even seniors) just take the wrong approach to a problem and a lot of time can be saved by just reviewing the code before it gets merged.
That's why we do code reviews. In fact, often we do design reviews too. We just have people tell us what they plan to do. Sometimes they have no idea, sometimes better ideas come out. But the key is to not waste days on lousy approaches and encourage discussions instead.
(All the linting and formatting and other stuff can be done by tools.. I agree on that point)
If I encounter an issue while doing a review, my response varies. Your approach is great, and it's what I do most of the time. If there's something really messed up that might require a gnarly explanation, I'll use another communication channel. I might call them up or arrange a zoom meeting. In a non-COVID world it would be an over the shoulder thing.
The goal is to discuss the issues in a way that doesn't make them feel like I'm calling them out in a code review comment. Then, we can discuss things in a way that allows me to use more contextual and emotional bandwidth than a black and white blob of text so that they go away feeling good about the discussion. Then, I'll go back to the PR and reject it with a comment that goes something like "Please make the changes that we discussed. The rest of this looks great!"
I've still gotten across everything that I needed to, and they don't feel like I shamed them in the permanent record. Obviously this isn't always possible, but it's helped avoid many potential issues.
One thing I always do when I think a piece of code could be better, is ask why they did it the way they. Or type what I had in mind and ask them what they think about it.
Reviews as discussions feel more welcoming, than reviews where someone is "correcting" you.
Exactly how I have been approaching code reviews for quite a while and the team happily follows this format. It's a conversation where you hope that there's learning involved. And it might as well be the reviewer who learns, even if he is in any form senior to the one who wrote the code.
I also try to throw in 'well done' remarks, to avoid it becomes a conversation only about the things that are so-so or are in need of a discussion.
I do not agree with some things you wrote. You write code for other people to understand. Machines will understand code way easier than humans as long as there are no mistakes in it and if there are, it won't compile and even come to a code review.
Your goal in a code review is, that when you have to add or fix something in half a year, that you understand the code fast and easy. So people write code for other people and when you think that most of your team save time to understand code when it uses a map instead of a for loop, then do it and change this in a code review. People should adapt to teams code styles so that you avoid mental load when reading code from other people on your team because everyone is doing things in a different way.
That's fine when there is a clear seniority difference, or when there is an obviously clearer way to write the code. I've usually worked on teams where most people are on a similar level and coding decisions are all about trade-offs / personal preferences. Bringing those types of things into a code review can be a great way to kill the team's dev speed for no obvious benefit
I can't agree more with you....Team must be on same page always...And Code review is one of the ways to do it
Great thoughts! What is your opinion on making comments to make code more concise but not exactly address a critical error?
For example, sometimes, I comment if someone is using a for loop when they could use a .map or .filter -> things that aren’t usually a big deal but could make it more readable for the next developer.
@jerielng Thanks, I'm glad this post could be useful! For cases like avoiding
forloops, I just use ESLint rules (and I also usehuskyandlint-stagedfor running both ESLint and Prettier as a pre-commit hook. It helps me to avoid all those nitpicking comments and keep the codebase consistent.But I think the most important thing is talking to your co-workers to see what kind of comments they like/dislike in code reviews.
I agree, that you do code reviews to avoid bugs. But the second important reason is to make sure, the code can be understood by other developers or the author after months. We write code for people and not for computers. ESLint, phpcs, phpmd, prettier, all of those and much more can help to avoid bugs and approve standards. But only a human can figure out, if an other human can read and understand the code.
To me, code reviews aren't about catching bugs introduced by a PR. Not at all.
That said if you spot a bug caused by the PR, you should still mention it. Obviously (:
The way I teach devs how to code review is that looking for bugs is the #1 most important thing. Detecting bugs during this stage is far cheaper than letting them seep out to our clients. Its even cheaper than letting it get to our test environment where QA has to find them.
Now something I clarify is that sometimes fragile code or things that are hard to read can lead to bugs even further down the line so those are important items to point out too. Readability and maintainability are words I used in A LOT of comments.
Of course, I'm happy and grateful if colleagues point out bugs in my PR before I merge it into master (and this saved me from embarrassment or causing troubles several times).
That said I don't expect colleagues to look for bugs or to catch bugs.
Why?
In the end, it depends on the team and the product/requirements. That's what has worked for me in the past. But that doesn't mean that I don't believe that your approach couldn't be the right one for me in some context.
In code review when looking for bugs, you just need to think about whether the right conditions are tested.
I think this is something that's working well in code reviews because of the different view when you haven't written the code yourself.
Also if you try to understand what the code does, often possible bugs become apparent almost automatically.
Just wanted to clarify, that there are of course good code reviews and bad code reviews and there are the right expectations/reasons to conduct code reviews and wrong expectations/reasons.
But it all depends on the context and even more importantly on the rest of the process. If you focus on one thing in code reviews the other point needs to be addressed at another point during the process.
What matters most (to me):
Code reviews need to be about support.
If there's a large PR, written by the most awesome, experienced 10x developer there's still a good chance even the intern, that is just starting out will be able to spot something.
It's sometimes hard if people point out a lot of things, it's easy to become defensive, maybe those people are actually less experienced or capable or couldn't write that code themself.
AWESOME (:
To me, that's the power of code reviews. Even some intern just starting to code has a good chance to improve the code written by the most experienced developer.
That's why code reviews are awesome and why they are so important.
I also wrote a similar list that could be used as complement of yours:
dev.to/sourcelevel/10-items-to-che...
I liked the example about async/await inside a loop.