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

11–20 of 104 posts

Re: Code Review Best Practices

#11

Post by Fr0styMatt8 »

hueving wrote: post #4
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 disagree. Breaking down a function into several one time functions that all sit at the same mental model of abstraction make code much easier to reason about.

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, SubfunctionC() might end up a few pages down in source code. So now it's a context switch to go there and then go back.

Now this can be somewhat avoided with good function names (so you don't HAVE to jump backwards and forwards) and keyboard shortcuts (to make the process quicker), but it's still a trade-off that I think needs to be kept in mind.

Re: Code Review Best Practices

#12

Post by azatris »

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.

Re: Code Review Best Practices

#13

Post by ams6110 »

Fr0styMatt8 wrote: post #11
hueving wrote: post #4

Earlier quoted context omitted.

I disagree. Breaking down a function into several one time functions that all sit at the same mental model of abstraction make code much easier to reason about.

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…

When I have to deal with something like this in emacs I will normally split the window in two so I can look at two different parts of the buffer at the same time. I assume most other editors allow the same?

Re: Code Review Best Practices

#14

Post by samspot »

Fr0styMatt8 wrote: post #11
hueving wrote: post #4

Earlier quoted context omitted.

I disagree. Breaking down a function into several one time functions that all sit at the same mental model of abstraction make code much easier to reason about.

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 you've hit upon a good measure of function quality. If you find you have to jump around a lot when reading, that would be a sign that it's poorly organized and needs to be refactored. On the other hand, if you find you don't have to jump around and can trust the sub functions by their names, then it's been broken up well.

In the best case you should be able to follow the logic without diving into the other functions, only looking at their implementation details as that particular detail becomes relevant.

Re: Code Review Best Practices

#15

Post by a3voices »

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.

Re: Code Review Best Practices

#16

Post by bozoUser »

While I agree with all the points in the blog, I wonder how many programmers do really follow them perfectly(even the author of the blog) because doing code review to such great detail requires plenty of time which is often not the case when you work for corporations.

Re: Code Review Best Practices

#17

Post by Kaedon »

bozoUser wrote: post #16

While I agree with all the points in the blog, I wonder how many programmers do really follow them perfectly(even the author of the blog) because doing code review to such great detail requires plenty of time which is often not the case when you work for corporations.

I'll admit that I don't follow them perfectly either. I'll usually pick one or two instances and work on those. I think most of these items are on a continuum from severe to minor basically. It's usually when it's on the severe end that I'll suggest it might be worth changing.

My code has bugs and flaws too! That's why I like code reviews, they give me an opportunity to see my blind spots.

Re: Code Review Best Practices

#18

Post by swsieber »

Fr0styMatt8 wrote: post #11
hueving wrote: post #4

Earlier quoted context omitted.

I disagree. Breaking down a function into several one time functions that all sit at the same mental model of abstraction make code much easier to reason about.

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 sane manner function calls, letting me recourse arbitrarily deep - some thing ala code collapsing and expansion. Extra points if you can show a for chart to side of function calls, class interactions and general code flow, highlighting where your cursor is. More points for a pane that shows what variables have to be in order for a the branch of code you're working in to be reached. A changeable granularity setting would be great for all of these.

I think often times we discuss pros and cons without discussing what they could be if we put some effort into changing the current situation.

Re: Code Review Best Practices

#19

Post by BurningFrog »

Fr0styMatt8 wrote: post #11
hueving wrote: post #4

Earlier quoted context omitted.

I disagree. Breaking down a function into several one time functions that all sit at the same mental model of abstraction make code much easier to reason about.

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…

Split into functions with clear tasks that have clear names and are relatively standalone.

If you have to jump around between functions a lot to understand something, the code probably could use some reorganizing.

Re: Code Review Best Practices

#20

Post by chunkstuntman »

bozoUser wrote: post #16

While I agree with all the points in the blog, I wonder how many programmers do really follow them perfectly(even the author of the blog) because doing code review to such great detail requires plenty of time which is often not the case when you work for corporations.

One company I worked for relied heavily on code reviews after every feature. At least two co-workers (one of whom had to be a supervisor) read, ran, and gave feedback on every piece of code. Reading others' code and providing feedback allowed me to improve my sight-linting ability, and it felt like each day my group's code as a whole was improving.

Having some accountability for writing sloppy code is very sobering.

Post reply on HN