August 6, 2012 at 7:57 am
brianpaulflynn (8/6/2012)
Providing positive feedback builds rapport that makes the constructive criticism a million times more valued.
Very good point, and well taken.
Hakim Ali
www.sqlzen.com
August 6, 2012 at 8:31 am
When developers are coding, most are doing what they think is their best. No one deliberately writes mediocre code, so reviewers need the tact and diplomacy of ambassadors. Few people have skin thick enough to absorb anything but the most constructive of criticism - and (that too) only when handled with kid gloves.
In my company I have not seen any evidence of code reviews (in the past three years) even though it's mentioned now and then in meetings as something that's supposed/expected to happen. I'm not sure how many companies enforce this as part of mandated quality control. In my experience, more time is taken defining and documenting a process than in any actual implementation.
Luckily, the Net is a great resource - we get to see/use code snippets posted for public consumption, forums such as ones on this site where people exchange their solutions to a problem - great articles with in-depth research (Jeff Moden always comes to mind here) - we get to see different perspectives and more importantly - learn why one solution is preferable over another.
If it were not for this, we would all have tunnel vision. I was lucky enough to briefly do some pair programming - it was an eye-opener in so many ways. I know it's not for everyone just as teleworking isn't, but if you pair up with the right person, it's invaluable. Jeff Atwood has a great blog about pair programming vs. code reviews - for anyone that cares to read it.
In the absence of any formal review process and even as an adoption of best practice, I have always learned to prefix my code with a meaningful comments block - whether it is to defend convoluted code or explain core logic - I find it helpful even when I myself revisit code after a long period of time - it helps me understand the reasoning at the time the code was written.
**ASCII stupid question, get a stupid ANSI !!!**
August 6, 2012 at 9:03 am
sushila (8/6/2012)
No one deliberately writes mediocre code
Yes, we all write code to the best of our ability. But the best of our ability does not always mean the best. If I have written code to the best of my ability, but somebody knows of ways to improve it, I would want to know that too so I can develop my own skills and improve the product for my employer. I see this as a valuable learning experience. If my best really is mediocre, I only stand to gain by constructive criticism. I'd be shooting myself in the foot and shortchanging my workplace if I said no to that.
Hakim Ali
www.sqlzen.com
August 6, 2012 at 9:37 am
hakim.ali (8/6/2012)
sushila (8/6/2012)
No one deliberately writes mediocre codeYes, we all write code to the best of our ability. But the best of our ability does not always mean the best. If I have written code to the best of my ability, but somebody knows of ways to improve it, I would want to know that too so I can develop my own skills and improve the product for my employer. I see this as a valuable learning experience. If my best really is mediocre, I only stand to gain by constructive criticism. I'd be shooting myself in the foot and shortchanging my workplace if I said no to that.
I couldn't agree more - I'm just calling for tact and diplomacy in dispensing constructive criticism and pointing out why (most) people get touchy & defensive (& on occasion even sullen)! 🙂
**ASCII stupid question, get a stupid ANSI !!!**
August 6, 2012 at 9:47 am
We review all of our code using a collaborative tool, which comes in handy if the author is in some other state or country (which is generally the case). A side effect is that it helps take a great deal of the emotion out of it because you are not sitting in a room with 3 or 4 other engineers meticulously going through the code. The tool gives the reviewers an opportunity to think about what what they are going to say and reduces some of the "knee jerk" responses you get in a live code review. It also allows the reviewers to work independently while still giving them full access to all comments and observations made by one another.
This doesn't mean that we don't have live input. If there is a tremendous amount of feedback, then the review can be escalated and we can either physically meet or have a conference call to discuss what has been captured using the tool.
If there is strong disagreement over style, structure or the algorithm, then we escalate the topic so that the entire (scrum) team can decide how to proceed. The idea is to determine if there is a need to establish a guideline or best practice or to improve the overall design or architecture.
In most cases, the author comes away from the review with their ego intact and their head held high because we are all focused on quality and are willing to take a step back whenever there is a difference of opinion. We also work hard to maintain a positive overall atmosphere whenever anything is reviewed, whether it is code or documentation.
August 6, 2012 at 9:51 am
One of the hardest things to learn is to separate your ideas from yourself, and to understand that criticizing your ideas is not the same as criticizing you personally. I think that is central to success at any level and in any field. In the context of a code review, this means (for the reviewee) being open to suggestion, understanding that the reviewers are discussing your ideas, not you, and going into the review with the attitude that a code review is a learning opportunity, not a trial. For the reviewers, it means understanding that you are reviewing code, not personality, that suggestions for change should be accompanied by explanations regarding why the change would be beneficial (something more substantial than "It's a best practice" or "That's the standard" is called for), and that, as was pointed out in an earlier post, it's always good to mix a little honey into the medicine to make it easier to swallow. We all like a pat on the back, after all.
Roland Alexander
The Monday Morning DBA
There are two means of refuge from the miseries of life: music and cats. ~ Albert Schweitzer
August 6, 2012 at 10:05 am
Roland Alexander STL (8/6/2012)
One of the hardest things to learn is to separate your ideas from yourself, and to understand that criticizing your ideas is not the same as criticizing you personally. I think that is central to success at any level and in any field. In the context of a code review, this means (for the reviewee) being open to suggestion, understanding that the reviewers are discussing your ideas, not you, and going into the review with the attitude that a code review is a learning opportunity, not a trial. For the reviewers, it means understanding that you are reviewing code, not personality, that suggestions for change should be accompanied by explanations regarding why the change would be beneficial (something more substantial than "It's a best practice" or "That's the standard" is called for), and that, as was pointed out in an earlier post, it's always good to mix a little honey into the medicine to make it easier to swallow. We all like a pat on the back, after all.
Yes and yes and yes. Like.
Hakim Ali
www.sqlzen.com
August 6, 2012 at 10:06 am
j_e_o (8/6/2012)
In most cases, the author comes away from the review with their ego intact and their head held high because we are all focused on quality and are willing to take a step back whenever there is a difference of opinion. We also work hard to maintain a positive overall atmosphere whenever anything is reviewed, whether it is code or documentation.
Truly the desired outcome.
Instead of brutally honest, I would call for being very honest, but also very tactful.
I suspect that what Hakim meant by brutally honest may not be how most of us (including me) tend to interpret it. Too often, I've seen "I was just being honest" used as an excuse to beat someone up verbally.
August 6, 2012 at 10:23 am
marcia.j.wilson (8/6/2012)I suspect that what Hakim meant by brutally honest may not be how most of us (including me) tend to interpret it. Too often, I've seen "I was just being honest" used as an excuse to beat someone up verbally.
Yes, I am discovering that the term has different connotations to different people. To help explain my stance on it, I am borrowing the following from another website (productiveflourishing):
By “brutal”, we don’t mean “cruel”. We mean honesty that tells it like it is — even when that truth is hard to speak, and hard to hear. Because bad advice, “nice” advice, can be utterly destructive.
If those bright pink jeans were pretty unflattering, wouldn’t you want to know before you went out on a date?
And if your business model was wonky, wouldn’t you want to know before you invested a ton of money and energy into pursuing it?
Hakim Ali
www.sqlzen.com
August 6, 2012 at 11:16 am
I have found that there is one thing that can be done to cause a more productive session. Lay out the ground rules and understandings up front. Stuff like this is not a rip session on individuals it is about making the code better, no person attacks if one starts the one attacking will be asked to leave the session, no editorializing with critical garbage thrown in just the cold hard facts, all discussion is for that session only and nothing goes out of the session except ways to make the system or code better.
With stuff like this understood up front by all the players things can progress. If then some stonewalling or issue rises you can refer back to the ground rules. If a developer starts to get defensive they can be reminded that this is not about them, it is about the project. If there is an argument it can not be managed as you vs me but one technical option or another.
Lastly, ownership of a piece of code or a project needs to be understood. I do not "own" the code I write. It is the property of my company or employer. I want the best for the company so they can make more money and I can benefit.
And lastly there is a maturity issue. Admitting one is wrong is hard for those who feel they still have to prove themselves. Admission of second rate or failure is weakness, and is not to be admitted. One who has fought the wars and bled in the battles knows though that to fail forward is progress, and to learn a better way is to achieve larger victories down the road.
I will now shut up and get back to it, The war rages on!
M.
And apologies if this has all been covered already. I did not have time to read everything this morning.
Not all gray hairs are Dinosaurs!
August 6, 2012 at 1:45 pm
hakim.ali (8/6/2012)If those bright pink jeans were pretty unflattering, wouldn’t you want to know before you went out on a date?
And if your business model was wonky, wouldn’t you want to know before you invested a ton of money and energy into pursuing it?
True, but your delivery of this message can determine whether the person is going to take your advice from then on or just ignore everything you say in the future. It's not the bad news, that is necessarily bad, it's how and when you deliver it. You have to use a "velvet hammer" at times with people, and you need to use good timing when you do decide to use it.. I found this out the hard way. 😀
"Technology is a weird thing. It brings you great gifts with one hand, and it stabs you in the back with the other. ...:-D"
August 6, 2012 at 2:02 pm
TravisDBA (8/6/2012)
True, but your delivery of this message can determine whether the person is going to take your advice from then on or just ignore everything you say in the future. It's not the bad news, that is necessarily bad, it's how and when you deliver it. You have to use a "velvet hammer" at times with people, and you need to use good timing when you do decide to use it.. I found this out the hard way. 😀
Yes, I think I am beginning to see that from the responses in this forum. Looks like not everybody is going to be open to listening to the message just because it is right. It also has to be delivered tactfully. Thanks for sharing your thoughts, it definitely helps me and hopefully others reading this thread.
Hakim Ali
www.sqlzen.com
August 6, 2012 at 4:19 pm
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.
August 6, 2012 at 4:23 pm
tomo.medhost (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.
Good thought... I will keep that in mind at future reviews.
Hakim Ali
www.sqlzen.com
August 6, 2012 at 5:31 pm
And Hakim, I must say that you are doing a good job of responding to the criticism that came out of this post.
Viewing 15 posts - 16 through 30 (of 43 total)
You must be logged in to reply to this topic. Login to reply