DEV Community

Discussion on: Mistakes I made in code reviews and what I do now

Collapse
davidsanwald profile image
David Sanwald

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 (:

Collapse
aaron_deming profile image
Aaron A Deming

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.

Collapse
davidsanwald profile image
David Sanwald

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.

Collapse
davidsanwald profile image
David Sanwald

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.

Collapse
davidsanwald profile image
David Sanwald

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?

  1. Catching bugs just by looking at the code takes a lot of time and focus. There's a class of bugs that is easier to spot if you didn't write the code, even if you just look at it. But really looking for bugs needs way too much effort and takes too much time for code reviews.
  2. I think there are a lot of techniques and practices to catch bugs, to me it's dangerous skipping on those because you assign responsibility to catch bugs to code reviews.
  3. While there are several effective techniques and practices to catch bugs, there aren't that many opportunities to learn from your colleagues, regardless if you learn from the way they see your code or you learn from the way you read/see their code.
  4. I want the main focus to be about checking if you could work with this code (or the coupling it introduces) in the future if somebody catches a bug, that's awesome but it's just a bonus/side effect.

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.

Thread Thread
zoechi profile image
Günter Zöchbauer

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.