6 ms·

A bit of a controversial one: give your reviewer one obvious (but minor) mistake to catch. For some reason, it feels really unsatisfying to a lot of people to c

A bit of a controversial one: give your reviewer one obvious (but minor) mistake to catch. For some reason, it feels really unsatisfying to a lot of people to conduct an extensive code review, and come up with exactly zero items to change. This is likely more relevant when you present something to a business leader who has the feeling they have to contribute something, but it can also apply to inexperienced code reviewers.

mtlynch 6y ago
I've toyed with this idea too, but for a slightly different purpose. When junior developers join my team, I find that they're often shy about critiquing anyone else's code because they assume they're the least skilled person, so they couldn't possibly find anything to be improved in anyone else's code. I've considered sending developers like this a code review and including the note, "And just so you feel comfortable suggesting improvements, I've intentionally left in one mistake (not necessarily a functional bug)." I've never done it because I worry it comes across as condescending. Like, "Obviously I'm perfect, but to give you something to do, I added a mistake, and now it's your job to go digging around for it." Instead, I try to reassure junior developers that they should feel comfortable making suggestions and asking questions in the code review, even if they're not finding things that are officially "bugs."
- alisonkisk 6y ago
  Just put the mistake in quietly, and if they miss it, notice it yourself later or ask someone else to comment on it
- Hello71 6y ago
  I think a change of framing might be helpful here. Put in multiple bugs, and call it "training" instead. If you have multiple junior developers, have them all do it and compare results, reinforcing the idea of multiple reviews.
- guytv 6y ago
  This is a very interesting idea to me. Over the years I noticed that reviewing the same code knowing a bug is there will increase my chances of finding it comparing to not knowing a bug exists. It would be interesting to measure if introducing a "catch" in every PR would increase the number of times the reviewer will find additional issues.
- Leherenn 6y ago
  I'm not sure that's particularly great for confidence. It will feel like a test, and they will feel dreadful if they don't find it. The whole process will be very stressful.
adamkl 6y ago
I'm not sure this is controversial so much as it is just terrible advice! Are you really suggesting that developers purposely introduce bugs into a code base just to keep reviewers on their toes? It may seem like a relatively harmless idea in the small, but in the large, imagine hundreds or thousands of developers taking this advice, and purposely committing bugs. Yikes.
- alisonkisk 6y ago
  No one said to commit bugs.
  - adamkl 6y ago
    If it's in a code review, it's been committed to some branch in source control. If it makes it past the code review, it's now a part of your product. If people want to use this approach as a training exercise (as mtlynch suggested in a sibling post), that's one thing, but that's not what OP suggested, and what OP suggested seems like pretty risky advice.
    - plorkyeran 6y ago
      If the deliberate mistake makes it past code review then you just remove it before merging anyway. A workflow where you are not allowed to fix anything in your own PRs before merging them unless they are explicitly requested by a reviewer would be very strange.
q3k 6y ago
> For some reason, it feels really unsatisfying to a lot of people to conduct an extensive code review, and come up with exactly zero items to change. You should fix that instead of trying to hack around this. Code review shouldn't be a nitpicking party for people that need to drum up their egos.
sixstringtheory 6y ago
Feels condescendingly manipulative ("I know your ego/self-esteem are low, let me throw you a bone"), and would just be more work for everyone; for them to articulate the problem they find, and regardless of whether they catch it or not, you have to now remember to go back and fix it before merging.

mtlynch 6y ago

I've toyed with this idea too, but for a slightly different purpose. When junior developers join my team, I find that they're often shy about critiquing anyone else's code because they assume they're the least skilled person, so they couldn't possibly find anything to be improved in anyone else's code. I've considered sending developers like this a code review and including the note, "And just so you feel comfortable suggesting improvements, I've intentionally left in one mistake (not necessarily a functional bug)." I've never done it because I worry it comes across as condescending. Like, "Obviously I'm perfect, but to give you something to do, I added a mistake, and now it's your job to go digging around for it." Instead, I try to reassure junior developers that they should feel comfortable making suggestions and asking questions in the code review, even if they're not finding things that are officially "bugs."

alisonkisk 6y ago
Just put the mistake in quietly, and if they miss it, notice it yourself later or ask someone else to comment on it
Hello71 6y ago
I think a change of framing might be helpful here. Put in multiple bugs, and call it "training" instead. If you have multiple junior developers, have them all do it and compare results, reinforcing the idea of multiple reviews.
guytv 6y ago
This is a very interesting idea to me. Over the years I noticed that reviewing the same code knowing a bug is there will increase my chances of finding it comparing to not knowing a bug exists. It would be interesting to measure if introducing a "catch" in every PR would increase the number of times the reviewer will find additional issues.
Leherenn 6y ago
I'm not sure that's particularly great for confidence. It will feel like a test, and they will feel dreadful if they don't find it. The whole process will be very stressful.

adamkl 6y ago

I'm not sure this is controversial so much as it is just terrible advice! Are you really suggesting that developers purposely introduce bugs into a code base just to keep reviewers on their toes? It may seem like a relatively harmless idea in the small, but in the large, imagine hundreds or thousands of developers taking this advice, and purposely committing bugs. Yikes.

alisonkisk 6y ago
No one said to commit bugs.
- adamkl 6y ago
  If it's in a code review, it's been committed to some branch in source control. If it makes it past the code review, it's now a part of your product. If people want to use this approach as a training exercise (as mtlynch suggested in a sibling post), that's one thing, but that's not what OP suggested, and what OP suggested seems like pretty risky advice.
  - plorkyeran 6y ago
    If the deliberate mistake makes it past code review then you just remove it before merging anyway. A workflow where you are not allowed to fix anything in your own PRs before merging them unless they are explicitly requested by a reviewer would be very strange.

q3k 6y ago

> For some reason, it feels really unsatisfying to a lot of people to conduct an extensive code review, and come up with exactly zero items to change. You should fix that instead of trying to hack around this. Code review shouldn't be a nitpicking party for people that need to drum up their egos.

sixstringtheory 6y ago

Feels condescendingly manipulative ("I know your ego/self-esteem are low, let me throw you a bone"), and would just be more work for everyone; for them to articulate the problem they find, and regardless of whether they catch it or not, you have to now remember to go back and fix it before merging.