Code Reviews Must Be Brutally Honest

Question

Post reply

Code Reviews Must Be Brutally Honest

hakim.ali

SSCarpal Tunnel

Points: 4239
More actions
August 5, 2012 at 2:43 am

#392893

Comments posted to this topic are about the item Code Reviews Must Be Brutally Honest
Hakim Ali
www.sqlzen.com

Viewing 15 posts - 1 through 15 (of 43 total)

You must be logged in to reply to this topic. Login to reply

mtucker-732014 SSC Eights! Points: 815 More actions · Answer 1

No need for brutality - you can be honest without being brutal

Core6430 Old Hand Points: 363 More actions · Answer 2

mtucker-732014 (8/6/2012)
No need for brutality - you can be honest without being brutal

I agree. Honesty is good. The article makes it sound like code reviews are supposed to be a blood bath. I'm hoping that what the author meant was that honest, helpful advice was given, but the reviewee got defensive over good criticism. Not that a bunch of his peers were put in a room to shred the poor guy from one end to the other disguised as "we're helping".

There's no reason for the person giving criticism to get "sullen" if criticism is not taken. That implies ego's ARE involved. Don't expect the person with the spotlight on them to react passively when confronted by 5 other people who are using it as a way to boost their ego's. Especially a group setting. That is a sure way to set someone up for defensive behavior.

hakim.ali SSCarpal Tunnel Points: 4239 More actions · Answer 3

Core6430 (8/6/2012)
mtucker-732014 (8/6/2012)
No need for brutality - you can be honest without being brutal
I agree. Honesty is good. The article makes it sound like code reviews are supposed to be a blood bath. I'm hoping that what the author meant was that honest, helpful advice was given, but the reviewee got defensive over good criticism. Not that a bunch of his peers were put in a room to shred the poor guy from one end to the other disguised as "we're helping".
There's no reason for the person giving criticism to get "sullen" if criticism is not taken. That implies ego's ARE involved. Don't expect the person with the spotlight on them to react passively when confronted by 5 other people who are using it as a way to boost their ego's. Especially a group setting. That is a sure way to set someone up for defensive behavior.

Thanks for replying. So many thoughts here:

I agree too, honesty is not just good, it is essential. Brutal honesty is not the same as brutalizing somebody, and it is not a blood bath either. What it simply is, is that if the reviewer thinks that there is a way to make improvements in the code, they should not hold back from dispensing that thought for no reason other than to be polite to the reviewee.

And yes, there is no reason for anybody in the review session to get sullen. Nor should the reviewee be passive in the face of constructive criticism: if they feel their code is right, by all means defend it. But defending something you believe in, and being defensive just to not be wrong, are two different things.

And yes on the ego boost too: just like it should not apply to the reviewee, it should not apply to the reviewer either. At the end of the day, it is about the code, not about you or me. That is why it must be honest.

Hakim Ali
www.sqlzen.com

Sergio Pines SSC Rookie Points: 35 More actions · Answer 4

Personally, I find two things show professional dedication: experience/knowledge and the ability to laugh at oneself; in the majority of cases the latter carries the most weight for me. I say this not to underscore once again what everyone here is saying, that the reviewee should act professionally and quit childish "it's my toy" reactions, but rather to point out that the reviewer should, too. Although in the majority of cases, these reactions come up in reviewees, unfortunately I have seen them crop up in reviewers as well - who insist on questioning issues that nobody cares the least about (because they are simply irrelevant); who seem to get a power-rush from being in a position to show off how far and how deep their Vision cuts (although it may appear pathetic to the rest). Curiously, when you poke at them there, their reactions are exactly the same as those of the childish reviewee: they throw a tantrum or blacklist you. Hence, I'd say that "the attitude" holds at both ends of the reviewer-reviewee relation. It means being as brutal with oneself as one is to others, in the same measure as one is able to fondly laugh about oneself as about others.

adlan SSC-Addicted Points: 420 More actions · Answer 5

I think 'frank and fearless' is likely to be way more constructive than 'brutal'. If you have a group of people in a room, all pointing out shortcomings in someones code it is pretty inevitable that they are going to get defensive, especially if they haven't had a chance to think about the criticism ahead of time. Code reviews are probably going to be more productive if you comment code ahead of time and give the person a chance to think about it before discussing it with them one on one. You can always bring in others if you can't agree. I suppose the format needs to be informed by the personalities of those involved.

Robert.Sterbal SSCrazy Points: 2855 More actions · Answer 6

How big is the organization you work for? This might not be an appropriate process for everyone involved. Do you have a manager that you both report to?

hakim.ali SSCarpal Tunnel Points: 4239 More actions · Answer 7

Sergio Pines (8/6/2012)
Personally, I find two things show professional dedication: experience/knowledge and the ability to laugh at oneself;
...
Hence, I'd say that "the attitude" holds at both ends of the reviewer-reviewee relation. It means being as brutal with oneself as one is to others, in the same measure as one is able to fondly laugh about oneself as about others.

Absolutely agree, well said.

Hakim Ali
www.sqlzen.com

hakim.ali SSCarpal Tunnel Points: 4239 More actions · Answer 8

adlan (8/6/2012)
I think 'frank and fearless' is likely to be way more constructive than 'brutal'. If you have a group of people in a room, all pointing out shortcomings in someones code it is pretty inevitable that they are going to get defensive, especially if they haven't had a chance to think about the criticism ahead of time. Code reviews are probably going to be more productive if you comment code ahead of time and give the person a chance to think about it before discussing it with them one on one. You can always bring in others if you can't agree. I suppose the format needs to be informed by the personalities of those involved.

Frank and fearless, or brutally honest, I think we are using different words to say the same thing. The point I am trying to make is that the criticism should be directed at the code, not the person. We are trying to improve the product/service, not prove anybody wrong.

Hakim Ali
www.sqlzen.com

hakim.ali SSCarpal Tunnel Points: 4239 More actions · Answer 9

Robert.Sterbal (8/6/2012)
How big is the organization you work for? This might not be an appropriate process for everyone involved. Do you have a manager that you both report to?

About 150 people at the local office. Yes, we do have a very competent manager.

Hakim Ali
www.sqlzen.com

Robert.Sterbal SSCrazy Points: 2855 More actions · Answer 10

I would encourage you to ask the manager to gentle find out if the code review process is a reasonable one for the developer who was struggling. And don't pester the manager for results or details. You might follow up with a question to the manager if it is appropriate at a future date to have a code review.

IowaDave SSCommitted Points: 1734 More actions · Answer 11

And, I think the reviewers need to remember that the reviewee has put his/her blood/sweat/tears into this code and it's therefore pretty natural to be a little defensive/guarded.

It's also helpful at the beginning of all code reviews to go over some of the points that Hakim raised here:

- we're here for the organization's benefit

- no need to be defensive ... or nit-picky

- we can all learn if we all participate

Thanks for the post!

hakim.ali SSCarpal Tunnel Points: 4239 More actions · Answer 12

Robert.Sterbal (8/6/2012)
I would encourage you to ask the manager to gentle find out if the code review process is a reasonable one for the developer who was struggling. And don't pester the manager for results or details. You might follow up with a question to the manager if it is appropriate at a future date to have a code review.

Thanks, I will consider taking this up with my manager.

Hakim Ali
www.sqlzen.com

Robert.Sterbal SSCrazy Points: 2855 More actions · Answer 13

hakim.ali (8/6/2012)
Robert.Sterbal (8/6/2012)
I would encourage you to ask the manager to gentle find out if the code review process is a reasonable one for the developer who was struggling. And don't pester the manager for results or details. You might follow up with a question to the manager if it is appropriate at a future date to have a code review.
Thanks, I will consider taking this up with my manager.

Please don't turn this into a demand. There are other ways to address poor code. Does the developer have a mentor? Does he generally do good work? Would the code reviews still be successful if he were out of the office on the day you had them?

brianpaulflynn SSC Journeyman Points: 98 More actions · Answer 14

Being honest in a code review is definitely needed! A manager, however, I often see offenses created in pursuit of noble "honesty". In most cases, these offenses could have been avoided had the revewer taken a moment to share not only negative feedback but taken some time to provdie some positive feedback. Let's not forget that reviews are as much an opportunity to compliment our fellow coders as providing constructive criticism. Providing positive feedback builds rapport that makes the constructive criticism a million times more valued. If you only go in with what you view as constructive criticsim without complimenting the things done well, you are almost asking for defensiveness.