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

31–40 of 104 posts

Re: Code Review Best Practices

#31

Post by userbinator »

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 or very similar to the others, then so be it.

I wonder what the distribution of function lengths is like in the Linux kernel... there will be longer ones and shorter ones, but a 20-line limit seems absurdly short.

Also vaguely related: http://www.satisfice.com/blog/archives/27

Re: Code Review Best Practices

#32

Post by matwood »

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.

Re: Code Review Best Practices

#34

Post by USNetizen »

The one thing that is missing, which ALWAYS seems to fall by the wayside, is security. If people incorporated more iterative security testing (static AND dynamic, automated AND manual) and threat modeling into their SDLC reviews there would be a plummeting number of vulnerabilities.

But, because it doesn't fit in with the whole "Lean" approach to software (deliver features yesterday), all but the most established enterprises don't seem to care much unfortunately. Once more people experience a breach because of their desire to deliver first and remediate vulnerabilities later then perhaps more awareness will be raised. By then it's too late though.

Re: Code Review Best Practices

#35

Post by arthurjj »

acveilleux wrote: post #27
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…

If you can't understand what UsedToBeSomeMassiveFunction() does from the SubFunction K ()'s names and arguments, they were not split out correctly. Often I find the best way to simplify large functions is to tear out sub-blocks and give them name. Loops or large conditional blocks are usually easy to tear out and can usually get very meaningful names. Long stretch of imperative code however does not separate well and…

"Long stretch of imperative code however does not separate well and probably should remain a very large function."

As someone who uses Resharper's Extract Method a lot this is a nice counterexample to where it's probably inappropriate

Re: Code Review Best Practices

#36

Post by allsystemsgo »

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 agree. I have had review comments that have made no sense at all. If I didn't ask why, I wouldn't learn. Also, to be honest, there have been times where the review comment didn't understand the context of my code, so the review comment ended up being incorrect.

Re: Code Review Best Practices

#37

Post by jlarocco »

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 nerf guns hinder development speed far more than code reviews.

Re: Code Review Best Practices

#38

Post by maguirre »

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?

Re: Code Review Best Practices

#39

Post by shakeel_mohamed »

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.
Post reply on HN