Code Reviews Must Be Brutally Honest

  • marcia.j.wilson (8/6/2012)


    And Hakim, I must say that you are doing a good job of responding to the criticism that came out of this post.

    Thank you, appreciate your saying it.

    Hakim Ali
    www.sqlzen.com

  • hakim.ali (8/6/2012)


    marcia.j.wilson (8/6/2012)


    And Hakim, I must say that you are doing a good job of responding to the criticism that came out of this post.

    Thank you, appreciate your saying it.

    I agree with Marcia.

    Were you as persistent and productive dealing with the other developer's criticism?

  • Great article! There was a time when I took stuff like this a bit too personally, but I am much more in tune with the process as a whole, rather than trying to protect or defend my contribution. 😛

  • tommy_o_is_my_name (8/6/2012)


    It really depends on the people involved, their relationship with each other and their understanding of each others personality. It's not what you say it's how you say it and to whom. From personal experience I have taken far more "brutal" criticisms from people I have great relationships with than "constructive" criticisms from people I don't.

    Thought about this, and have to ask: if you are being objective, then does it matter who is delivering the message so long as it makes sense? If someone you dislike points out something you agree with on a logical level, something you know deep down inside is "right", will you still refuse to accept their input? It might be hard, but I think I would want to accept what is being said, to try and divorce the (good) message from the (bad) messenger.

    Hakim Ali
    www.sqlzen.com

  • If someone I "dislike" points out something that I know is wrong than of course I'll change it but what's important to understand is that we work with computers, we aren't computers. Our ability to communicate effectively and maintain (at the very least) professional relationships with others is what leads to our success.

  • I recommend that you read "5 dysfunctions of teams" which covers the topic of honesty, trust, ego, results and conflict.

    When I was a lead DBA I always gave my code to two particular guys because I knew they would always find something in the code to critique. The most dangerous thing as a lead is for rank/status to get in the way of quality.

    I'm a bit of a perfectionist so it was never a comfortable experience to have someone find a fault in my code but I knew it had to be done and the end result was a higher level of quality. Those two particular guys took the same approach.

    Conflict is good because it thrashes out the disagreements and you end up with a resolution to which people feel comfortable signing up to. If you avoid conflict then (for want of a better description) the "grudge" festers and you get underground sniping at solutions.

    We almost came to blows several times because we were/are passionate about our profession, but ultimately we have the heartiest respect and trust for each other because of it. We recognise each others professionalism.

  • David.Poole (8/10/2012)


    I recommend that you read "5 dysfunctions of teams" which covers the topic of honesty, trust, ego, results and conflict.

    Thank you for your thoughts, and for the recommendation, I will try and read it.

    Hakim Ali
    www.sqlzen.com

  • Quite frankly, in my experience with agile techniques, particularly SCRUM, the methodology negates most conflicts regardless of titles or roles.

    Everyone has the same role: team member. That's it. Everyone has the same responsibilities: contribute to the success of the sprint. Work is picked up as necessary and the team dynamics prevent anyone from not pulling their weight, since it becomes painfully obvious on a daily basis at the very least.

    The more seasoned team members have to step up when the going gets rough because that is what is expected. Less experienced or technically savvy team members learn and improve when the heavy hitters help out because mentoring is also part of the latter's job description and part of their responsibility to the team.

    Not only that, but we have more code to review than most: production code, unit test code and subsystem test code (we have a very high percentage of test automation...very high). We all pitch in to get it done right. (Yes, the devs help QA without being pushed, shoved or threatened.)

    We just completed a 2+ year TDD/CI/SCRUM project that was a huge success. Not only that, but we were the pioneering project with agile + SCRUM (4 distributed teams no less with 40+ participants across 3 time zones). No outstanding bugs and the customers are head over heels in love with the product.

    Of course, to really screw up a project, just do agile + SCRUM badly. Do it right and it is amazing how well it works, even in code reviews.

  • j_e_o (8/10/2012)


    Quite frankly, in my experience with agile techniques, particularly SCRUM, the methodology negates most conflicts regardless of titles or roles.

    Yes it does, but getting the culture of the business to shift is the tricky bit. The initial stages of adopting SCRUM are quite a culture shock.

    Getting the small quiet guy in the corner to shout out that the big noisy guy at the front is on the wrong track is a perenial problem.

  • The more senior people draw out the more quiet folks by gently but firmly encouraging them to participate based on what they know to be their strengths. So in any technical discussion, I would ask for the quiet person's thoughts by invoking some of their recent work or technical achievements in terms of the problem being addressed. Works more often than not and eventually is no longer necessary because the "quiet" ones realize that they do have something positive to contribute and that it is expected of them.

  • Great topic and article! Thanks. I think it's worth repeating that there is nothing wrong (in general) with making a mistake, it's how we learn. It's a problem when you keep making the same ones. I would also add that you should leave your stylistic biases at the door in a code review. They add little if anything to the process. If you sincerely want to improve yourself then you should learn to welcome constructive criticism no matter what your initial feelings may be during the review. I think another name for it is 'professionalism'.

  • cdonlan 18448 (8/14/2012)


    I think it's worth repeating that there is nothing wrong (in general) with making a mistake, it's how we learn.

    ...

    If you sincerely want to improve yourself then you should learn to welcome constructive criticism no matter what your initial feelings may be during the review. I think another name for it is 'professionalism'.

    Thanks, and well said. I agree. As for being wrong, take a look at this[/url].

    Hakim Ali
    www.sqlzen.com

  • Code reviews are interesting. In my experience, people that are defensive can often be "turned around" in their thinking if they can be made to realise that the code belongs to the company not to them. It is not their property and so everybody involved should be allowed to make suggestions to improve it if they can.

    Some people do find it difficult to say "er, actually you're right"

    Tony

  • tsmall 52653 (8/16/2012)


    Code reviews are interesting. In my experience, people that are defensive can often be "turned around" in their thinking if they can be made to realise that the code belongs to the company not to them. It is not their property and so everybody involved should be allowed to make suggestions to improve it if they can.

    Some people do find it difficult to say "er, actually you're right"

    Tony

    Excellent point! As I said earlier in the thread, it can be hard to separate who we are from what we do. Remembering that the code we produce is a product created for the benefit of our employers is a big step in the right direction!

    Roland Alexander 
    The Monday Morning DBA 
    There are two means of refuge from the miseries of life: music and cats. ~ Albert Schweitzer

Viewing 14 posts - 31 through 43 (of 43 total)

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