Code Review Best Practices
kevinlondon.com
Code Review Best Practices
1–10 of 104 posts
Re: Code Review Best Practices
#2setting 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...
Re: Code Review Best Practices
#3Awesome. 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!
Re: Code Review Best Practices
#4setting 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.
Re: Code Review Best Practices
#5setting 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...
Where I work we refer to guidelines like this as code smell. These are cues that refactoring should be considered but not strict rules which would prevent a commit.
Re: Code Review Best Practices
#6setting 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 actually improves readability and maintainability of the code.
Re: Code Review Best Practices
#7setting 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.
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.
Re: Code Review Best Practices
#8setting 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...
Yeah, thank you for the link! I use this list as a set of rough guidelines, basically, so it may or may not apply in all cases.
Sometimes it can be helpful to split up a method to be called once if it helps readability, for example if it helps split up a loop or abstracts the specific way we get a value.
Re: Code Review Best Practices
#9setting 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...
Where I work we refer to guidelines like this as code smell. These are cues that refactoring should be considered but not strict rules which would prevent a commit.
Yes, exactly true. There are situations where it's probably fine to violate the guidelines if there's a good reason.
Re: Code Review Best Practices
#10Awesome. 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!
Sure! Best of luck at Cisco. I think everyone looks for different things in a code review, this is just what has worked for me.