Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Code Review Best Practices

kevinlondon.com

Post reply on HN
Print view View original

61–70 of 104 posts

Re: Code Review Best Practices

#61

Post by krzyk »

enqk wrote: post #2

setting thresholds on method / class sizes seems quite arbitrary and potentially harmful. Splitting a method into n different ones, none of which is called more than once is setting up the code for opportunistic reuse and obscures it's true function. It's especially wrong if the code that was split is mutating / non pure functional. see http://number-none.com/blow/john_carmack_on_inlined_code.htm...

If you name the methods correctly (what they do) splitting actually helps readability of the code. It is quite hard to read and remember what was in the beginning of 500 line method.

Re: Code Review Best Practices

#62

Post by OnlineRevenge »

"Like anybody would be, I was very skeptical about using a love spell or any spell for that matter but I was absolutely shocked when Tim called me after I had you guys cast my "Return My Lover" spell for me. It wasn't 24 hours that I had my spell cast that he came back to me (practically on his knees). He broke up with me over a month ago and now we are happier than ever. Thank you all!" quickrevengespell@yahoo.com, Dorothy Rodriquez, New York

Re: Code Review Best Practices

#63

Post by OnlineRevenge »

I want to testify of this great death spell caster. This great man helped me cast a death spell on my wicked step father and just within 48hours the wicked man had a motor crash and died. All thanks to this great death spell caster called instant death spell. You too can contact him now for an urgent death spell cast on anyone, quickrevengespell@yahoo.com,

Re: Code Review Best Practices

#64

Post by OnlineRevenge »

"Like anybody would be, I was very skeptical about using a love spell or any spell for that matter but I was absolutely shocked when Tim called me after I had you guys cast my "Return My Lover" spell for me. It wasn't 24 hours that I had my spell cast that he came back to me (practically on his knees). He broke up with me over a month ago and now we are happier than ever. Thank you all!" quickrevengespell@yahoo.com, Dorothy Rodriquez, New York

Re: Code Review Best Practices

#65

Post by OnlineRevenge »

"I missed my ex bad. My family and friends were tired of me being so upset one of them actually ordered a Love Spell for me From Extreme Spells I had no idea what they had done. They ordered the GLOBAL LOVE SPELL as it your best and most powerful and effective Love Spell. Needless to say, I was shocked to see my wife at the door a week later with her eyes full of tears, .I cannot believe how well my spell worked. I recently ordered a Money Spell because who doesn't need extra money?" quickrevengespell@yahoo.com -- William, Nashville

Re: Code Review Best Practices

#66

Post by cpitman »

maguirre wrote: post #38

I have an interesting problem. A co-worker of mine appears extremely sensitive to his code being reviewed and I honestly don't know how to deal with it. He feels attacked (and becomes defensive during code reviews) because the reviewers focus on the "bad things and mistakes" of his code instead of the accomplishments. Has anyone here dealt with similar issues during reviews?

I'm assuming you are performing asynchronous code reviews (like pull request comments). If you have the time, one of my teams did occasional "Code Inspections", where a team of 5 people would review a larger chunk of functionality together.

The key is that each person has an assigned responsibility, and that one of those is the "Reader". The reader presents the code for review, and the reader is NOT the author. The author is there, but I think it helps when someone else is presenting the code to reduce the feeling that the review is of the author.

Re: Code Review Best Practices

#67

Post by tomjen3 »

maguirre wrote: post #38

I have an interesting problem. A co-worker of mine appears extremely sensitive to his code being reviewed and I honestly don't know how to deal with it. He feels attacked (and becomes defensive during code reviews) because the reviewers focus on the "bad things and mistakes" of his code instead of the accomplishments. Has anyone here dealt with similar issues during reviews?

Sounds like he is too attached to his code - maybe make it abundantly clear that you are critiquing his code not him. If you have the ability to do so, try to have him fix bugs in other peoples code (that way it might feel less like his code).

Re: Code Review Best Practices

#68

Post by maguirre »

MaulingMonkey wrote: post #47
maguirre wrote: post #38

I have an interesting problem. A co-worker of mine appears extremely sensitive to his code being reviewed and I honestly don't know how to deal with it. He feels attacked (and becomes defensive during code reviews) because the reviewers focus on the "bad things and mistakes" of his code instead of the accomplishments. Has anyone here dealt with similar issues during reviews?

Balancing the critique with some positive feedback - parts you liked, appreciation for the features shipped, etc. - is one idea I've seen floated around, for a particularly touchy coworker. Also, generally managing tone - "Looks good! Don't forget to add the doc comments on functions X Y and Z" vs "Why are X Y and Z missing doc comments?". Both have the same fundamental information (X Y and Z could use doc comments),…

Thanks for the feedback. We are a small team (3 embedded sw developers) and I am more or less the guy with actually production releases under my belt. I feel my approach has been very measured and very polite to the point where I feel like i am walking on "egg shells" when doing the code reviews.

Show them you get things pointed out in your own code reviews, that it's a team game where everyone is just looking out for each other. That bad code doesn't mean they're a bad programmer, that critique of their code isn't a critique of their skill, that everyone gets blind to the problems in their own code and benefits from a second set of eyes.

within a few days we're going to be trying exactly this in hopes to show how code reviews are not about attacking and instead about helping.

Re: Code Review Best Practices

#69

Post by tomjen3 »

matwood wrote: post #32
a3voices wrote: post #15

Having worked for businesses that use code reviews and those that don't, I personally favor not having code reviews. The reason is that they hinder development speed quite a lot, since you have to try to predict what other engineers will say on your reviews, which takes a lot of brainpower.

> since you have to try to predict what other engineers will say on your reviews So the code is written with higher quality up front? Sounds like reviews are working as intended.

If so, don't you think that is what he would have written?

There are enough differences over what can be good quality that teams can be bogged down over discussions that just have no good answer and these take a lot of brain power.

Re: Code Review Best Practices

#70

Post by dewiz »

maguirre wrote: post #38

I have an interesting problem. A co-worker of mine appears extremely sensitive to his code being reviewed and I honestly don't know how to deal with it. He feels attacked (and becomes defensive during code reviews) because the reviewers focus on the "bad things and mistakes" of his code instead of the accomplishments. Has anyone here dealt with similar issues during reviews?

Try with design sessions upfront, i.e. discuss how the problem should be solved in terms of patterns and architecture. Once that is agreed, with 2-3 people, then the code review becomes simply a matter of style and there you can only invite for consistency with the rest of the code base. Often the problem is in the tools, face to face conversations, pair programming, human interaction help with that. You can also have your colleague help with a task of yours and show that you appreciate his input.
Post reply on HN