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

91–100 of 104 posts

Re: Code Review Best Practices

#91

Post by nazbot »

maguirre wrote: post #68
MaulingMonkey wrote: post #47

Earlier quoted context omitted.

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 fo…

It's a problem for small teams and people with little experience with code reviews. The 'walking on egg shells' thing happens ALL the time.

I've never seen this get solved overnight. Your management really needs to sit down and explain their expectations around code reviews - that fixing things based on co-worker suggestions is a positive thing, that having negative comments won't affect their performance reviews, etc.

I also find that with people like that you really do need to let more things go. If it's not a critical bug or an actual problem with the code then let it slide. Some people just cannot take criticism (even constructive) and the bad blood you'll get by persisting with it will not help you.

I also tend to find these developers are not necessarily the strongest devs and often have other personality problems. This likely indicates that your company needs to focus a lot more on the hiring and culture.

Re: Code Review Best Practices

#92

Post by nazbot »

trustfundbaby wrote: post #30

> If the reviewer makes a suggestion, and I don’t have a clear answer as to why the suggestion should not be implemented, I’ll usually make the change This I feel is bad. Code reviews are usually between peers so you shouldn't be afraid to seek out clarification where possible. You shouldn't be making edits to code that goes in production without clearly understanding why. The other thing that wasn't mentioned, that…

I'm going to differ on the 'don't be a blocker' thing.

It varies by team. If your whole team agrees that the code reviewer is the 'gate' then people understand that they just need to fix the thing and move on. I personally love that style of working because you tend not to write trivial suggestions and everyone knows that what people have written isn't meant as nit-picky and just has to be changed.

If you don't think it's the right change then the other person can just ask why it's important and hopefully get educated. I also tend to just trust my colleagues enough that what they suggest is a good change. I often find I may miss small things through like lack of documentation or whatever (get tired after a while) and those reminders in the CR are the equivalent of a spotter getting you to do one last rep at the gym.

Re: Code Review Best Practices

#93

Post by zatkin »

azatris wrote: post #12
zatkin wrote: post #3

Awesome. I'm joining Cisco for the summer, so I think this would help me get a head start since they do code review. Thank you!

Are you the person who took my place? :) Got to the last stage, twice, in London. To me, the Code Review Best Practices seem awfully like very general knowledge, just gathered together. Not sure if it's HN-worthy per se. However, the John Carmack link is quite enlightening.

I sure hope not -- I'm working at the headquarters in San Jose this summer.

Re: Code Review Best Practices

#94

Post by zo1 »

userbinator wrote: post #31
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...

Agreed, I find it difficult to read code that's been split up into many small pieces because it gets difficult to keep track of the overall flow. Even with an IDE, the jumping around (and maintaining a mental call-stack) can be quite distracting. My guideline is basically "as big as it needs to be, without having large pieces of duplicated functionality." If an algorithm requires 100 lines, none of which are repeated…

You're not supposed to "flow" through multiple levels of abstraction in your head. That is the fundamental problem, and why we so often think that it's okay to have large functions (you know, to 'keep it all in our heads', or whatever reason is most often touted).

Not only that, but as the OP mentioned that it doesn't work if the methods/functions have side-effects, then we're building up opinions on a sloppy and fundamentally broken house of cards.

First off, your methods should not be having side-effects that are not obvious from the purpose of the method. And then secondly, back to my original point about not having to flow through the code. In light of that, a function should do one thing, and that's it. And when it does so, the method name clearly states what it does. After you put those two together, you simply "assume" that method does what you expect it to, and "flow" through the logic at a higher level.

I'm not going to go down into some silly "read_input_data" method while "flowing" through the high-level code, because I know what it is supposed to do. And by extension, at some point I will review it in its singular/atomic entirety, or would have already done so. Either read_input_data returns the data, or it loads it up into a instance-variable.

Re: Code Review Best Practices

#95

Post by ThrustVectoring »

bsdpython wrote: post #79
ThrustVectoring wrote: post #7

Earlier quoted context omitted.

A named one-time-use function has a name that describes what it does. This seems obvious, but bears repeating: instead of having to figure out what a chunk of code does, you can guess from the name and have a series of steps naturally described.

You can get the same effect with a block of code by properly naming variables and adding a single comment line if need be. It's really just a personal preference.

You'd have to have a standard vim config with folding to get it exactly the same.

Also, there's more restriction in that the single-use code block only has access to the variables passed into it, which makes it easier to parse, since less things could be happening. It serves to prune variables from the local scope.

Re: Code Review Best Practices

#96

Post by OnlineRevenge »

HERE COMES THE MASTER OF DEATH SPELLS. THIS GREAT DEATH SPELL CASTER CALLED "REVENGE DEATH SPELL" IS TRULY A GREAT DEATH SPELL CASTER INDEED.HE HELPED ME CAST A DEATH SPELL ON MY WICKED AND HEARTLESS EX HUSBAND AND JUST WITHIN 24HOURS THE BASTARD WAS CONFIRMED DEAD IN HIS SLEEPING BED. IF YOU ALSO NEED AN URGENT DEATH SPELL ON ANYONE THAT CONTACT THIS GREAT DEATH SPELL CASTER IMMEDIATELY VIA EMAIL, quickrevengespell@yahoo.com, website: www.quickrevengespells.com

Re: Code Review Best Practices

#97

Post by jbbarth »

enqk wrote: post #86
mikehaggard wrote: post #82

Earlier quoted context omitted.

>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. Sometimes, but in general I don't agree. If the functions are made private and giving appropriate names than it's really a way of self-documenting code, which helps. Su…

This depends on the language capabilities. In the C/Lisp/ML families of languages at least you can create isolated scopes. It's unfortunately not true of Javascript (and Ruby?).

In Ruby some people achieve this by wrapping actions in their own class, which does the trick if the classes are kept independent from each other (no shared state, except global vars, which nobody uses in the ruby world).

Re: Code Review Best Practices

#98

Post by enqk »

jbbarth wrote: post #97
enqk wrote: post #86

Earlier quoted context omitted.

This depends on the language capabilities. In the C/Lisp/ML families of languages at least you can create isolated scopes. It's unfortunately not true of Javascript (and Ruby?).

In Ruby some people achieve this by wrapping actions in their own class, which does the trick if the classes are kept independent from each other (no shared state, except global vars, which nobody uses in the ruby world).

I should have been clearer. I meant isolated scope within a function.

Re: Code Review Best Practices

#99

Post by motti »

AlisdairO wrote: post #76
krzyk wrote: post #61

Earlier quoted context omitted.

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.

IE it can help to understand the intent and flow of the code, but also make it harder to debug. If I'm in a situation where I want to follow the exact instructions a large fraction of the code performs, a call stack jumping all over the place can be pretty frustrating.

Step over

Re: Code Review Best Practices

#100

Post by avn2109 »

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…

You have suggested so many wins here. Gallons of win.
Post reply on HN