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

51–60 of 104 posts

Re: Code Review Best Practices

#51

Post by hliyan »

shakeel_mohamed wrote: post #39
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?

+1 I'm currently dealing with this. The individual doesn't offer feedback on my code when under review, so it appears like a personal attack.

We do the bulk of our reviews at the pull request stage, as line comments on commits (we use Github). These are visible to all team members. This form allows developers to take the time to phrase the feedback diplomatically.

I've also set the tone early on in my own comments (I run the development division) and I've encouraged the more experienced and talented members of the team to review my own pull requests. When they spot a problem in my code, I always make it a point to praise them (if, of course, the problem is valid). This seems to have worked.

Re: Code Review Best Practices

#52

Post by Cymen »

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?

Have you considered going out to lunch with them or grabbing a coffee and bringing it up? If you're in an organization where you wouldn't be comfortable with that, maybe talk to your manager and ask them about it? Basically, we all have to toughen up a little bit when getting reviewed and it is for the good of the project/team. So getting someone who is uncomfortable past that is a win-win for everyone.

Re: Code Review Best Practices

#54

Post by MaulingMonkey »

jlarocco wrote: post #37
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.

Also having experienced both, I wouldn't want to work in a place that didn't have code reviews. I ask myself, "WTF was this guy thinking when he wrote this?" far less often when I'm working on code that's been code reviewed. Most of the time, the reviewer catches code like that, and requests that it be fixed and/or commented, before I end up debugging through it 6 months later. IMO, open office plans, reading HN and…

I experienced both within the same company - they started without code reviews, and the later introduced them. The result was overwhelmingly positive, greatly improving code quality and helping spread knowledge of new systems and utilities.

I certainly believe it's possible to do code reviews sufficiently "wrong" that they're a net hindrance. Drape them in too much ceremony, put too much emphasis on style (Bob doesn't like your whitespace preferences, and he's the code owner, so your changelist is vetoed!) and too little emphasis on subsistence (these corner cases and possible bugs concern me).

.

We went with a relatively light touch approach. Nothing was technologically enforced - our lead dev simply told us he wanted everyone to start having everything reviewed by anyone - both for code quality and to spread knowledge - and that he'd be pissed if he found a bug in one of our changelists and found out it hadn't been reviewed, going forward.

Whitespace / naming stuff I tended to submit without review.

Quick and obvious fixes, small changes, etc., I was fairly willing to start a review request, submit, and then fix anything caught in the review in a followup changelist.

For larger changes where I had more reservations, I'd usually hold off on submitting until after a review. Maybe threaten to submit if they were dragging their feet too long :P

Re: Code Review Best Practices

#55

Post by supercanuck »

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 a compliment sandwich... Sandwich the criticism between two positive statements.

e.g. I like the way you did this, but I think this would work better but you're on the right track.

Re: Code Review Best Practices

#56

Post by yoklov »

reipahb wrote: post #6
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...

I agree that arbitrary limits can be a bit restrictive at times, but that is mostly an issue when dealing with automated code checking tools. However, in this case it is more about something to keep an eye for for the reviewer -- if the function is large and complicated it may be a good idea to take a closer look at it. Presumably the reviewer won't force the function to be broken into smaller pieces unless it actual…

Its extremely subjective though. For the first few years after starting, I thought that large functions were bad and should be split up unless theres a clear reason not to.

Now, I feel that it should be the other way. Most of the time you want to keep the code that does something together, splitting it up only to factor out common reusable pieces (that are actually reused).

This is also discussed in the linked article.

Re: Code Review Best Practices

#57

Post by [deleted] »

swsieber wrote: post #18
Fr0styMatt8 wrote: post #11

Earlier quoted context omitted.

I agree with you in principle but find that code editors let me down in that regard. Say I split a function up into a few sub-functions because it's getting big. Now I have the problem that I'm jumping forwards and backwards through the code when I want to explore what that function does: SomeMassiveFunction() { SubfunctionA(); SubfunctionB(); SubfunctionC(); SubfunctionD(); } SubfunctionA() { } .... In this case, Su…

I think it's a trade off we could get away from if we improved our tools. We should make tools that reduce our cognitive over head. I am regularly diving through code at my new job trying to figure out how everything fits together. Reading code split between various functions and figuring out program flow is a necessary skill, even if you do keep monolithic functions. I would love an IDE that showed inline in some sa…

[deleted]

Re: Code Review Best Practices

#58

Post by curun1r »

supercanuck wrote: post #55
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 a compliment sandwich... Sandwich the criticism between two positive statements. e.g. I like the way you did this, but I think this would work better but you're on the right track.

The feedback sandwich is something to be avoided. People are generally predisposed to either hear positive or negative feedback, but not both. When you give sandwiched feedback, those that easily hear positive feedback will miss the negative and those that easily hear negative feedback will miss the positive. Almost no one will hear both.

Or so says the management training courses I've recently taken. Supposedly there's studies to back this up, too, though I've never read them myself.

Re: Code Review Best Practices

#59

Post by bottled_poe »

Some of this is frustrating to read. Architectural and detailed design decisions should be made and approved almost entirely before the coding of those features is started. (Obviously this is the ideal and not always possible).

This means that the code reviews should involve little more that a checklist of those approved design decisions against their implementation, perhaps a code style is verified as well.

Coding without a design is just hacking, which I believe is the primary cause of burn-out and should be avoided as much as possible.

So, the question is, do you have a design document? I know it doesn't sound very agile, but traditional engineering procedures, when managed well, have a lot of merit in controlling product quality and cost.

Re: Code Review Best Practices

#60

Post by garthk »

MaulingMonkey wrote: post #49
halostatue wrote: post #46

Earlier quoted context omitted.

Go have coffee (or whatever) with your colleague and have a 1-to-1 conversation with them saying you want their feedback on code. You want their expertise to make you a better software developer. Even if you are better at this than they are, you can learn something from the sharing. The team’s lead should make sure that the individual in question knows that software development is a cooperative process and that code…

This. Although I'm less civil about it - I'll start giving people shit for rubber stamping my code reviews. "Why are you letting me ship this terrible code I wrote? Do you want us to end up in a death spiral of technical debt and crunch? Don't be an asshole, critique my shit! We all need a second set of eyes - I'm no exception!" ...okay, I'm maybe a little more civil than that. But I'm not above leaving some mistakes…

Ha! I'm like that with my QA people, begging them to come after me with a blunt weapon.
Post reply on HN