I recently came across these tweets from David K. Piano about code reviews:
// Detect dark theme
var iframe = document.getElementById('twee...
For further actions, you may consider blocking this person and/or reporting abuse
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.
I really appreciate your article as I always find it difficult to do code-reviews....
Usually, I have to review my co-workers/FRIENDS code so it's hard for me to mention the situations that the code is not that good/could be improved...
I only partially agree with this. I do agree that coding style should be automated and personal implementation preferences not discussed. But a code review is a great first test, if another developer understands your code. This is important and should be discussed.
This is exactly how I've been feeling last weeks.. But, to be fair, I've learned a better way to do things.
Great article!
Nice post , how you manage your time to respect all what you explain here? because I think it needs time to search best explanations for every case
@kamo Thanks! I'm glad you liked it! As @djamaile mentioned, I usually block a couple of hours per day for code reviews. But with time, some things become easier and more intuitive to see, so it's not that time-consuming anymore. By not nitpicking, we actually save time on code reviews.
Tbh, I usually don't do that much research seeking "the best explanation". If I'm not seeing a bug, I just trust my colleague has done their job well. If there's something that requires more research, I just point out some possible issues, so we can brainstorm together. I think having an honest, transparent conversation is the key here.
Seniors get more time to do code reviews. Some even block time to do only code reviews. At least, that’s how I experienced it.
Agreed! I follow the same philosophy and got great results out of it!
Love this. Nicely done.
Great article! Thank you for sharing.
Your post is exactly what my team-lead tells me)