This is not a bad list, and can be useful as a checklist.
But note that it basically tries to define good programming practice in general. That's a very big topic with a lot of room for debate and disagreement.
Code Review Best Practices
21–30 of 104 posts
Re: Code Review Best Practices
#22Nice list. This is more or less what I look for. It's nice to see your rules of thumb.
Anyone have any suggestions for time-estimating code review? That's the biggest issue we've faced trying to implement code review into our workflow.
Re: Code Review Best Practices
#23Nice list. This is more or less what I look for. It's nice to see your rules of thumb. Anyone have any suggestions for time-estimating code review? That's the biggest issue we've faced trying to implement code review into our workflow.
SmartBear has their own set of best practices and they recommend about 300-400 loc per hour[1]. I think that's probably about right. It's something I think I've gotten faster at over time, partially because we're improving from previous reviews and partially because it's a skill that develops.
[1]: http://smartbear.com/smartbear/media/pdfs/wp-cc-11-best-prac...
Re: Code Review Best Practices
#24Having 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.
I think code reviews are critical for sharing knowledge on team projects. It helps keep your team informed about refactors and new functionality, while also giving a space for feedback on implementation (in a critical time, before the code has shipped). It allows a very organic way for people to learn from others (reading code, asking about code, thinking about others' code).
That said, you have to get useful code reviews to see any benefit. To me that means using automated tools to do most of the style checks (your braces should be on this line, no space after this foreach, etc) and having an active culture of not being human-powered code linters when doing code reviews. There is a lot of work that goes into having a team give effective code reviews.
I agree that code reviews can slow down an individual, but the speed up to the team through shared understanding should make up for that.
Re: Code Review Best Practices
#25[deleted]
Re: Code Review Best Practices
#26[deleted]
[deleted]
Re: Code Review Best Practices
#27Earlier 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…
If you can't understand what UsedToBeSomeMassiveFunction() does from the SubFunctionK()'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 probably should remain a very large function.
Re: Code Review Best Practices
#28While 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.
> supervisor ... ran ... code
This and this again. Engineering managers, and all that. But also, don't just review, 'sight-lint', and reason about code. Rather, run the tests! It's the shared accountability and knowledge. Does the baseline capability exist (tests pass)? Can you, the reviewer, spot fallacies that the tests don't capture (if not, criticisms feed back to you later)?
Re: Code Review Best Practices
#29setting 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 mean, some functions/methods will be longer than usual. Even if you split it into multiple ones the end product will be the same. Due to some of those derivative functions/methods being exclusive children to the caller. Sometimes it's best to leave the bigger one alone, and some times it helps to break it down. You do end up with basically the same size codebase. Dunno if it's more readable. Though this somehow shines some light into other issues. Such as language verbosity. Some languages are just syntax factories. All the logic is accompanied by a truck load of verbosity.
I agree with your point. It seems silly to be strict about such things. More so if the codebase is written in Java or any other verbose language.
Re: Code Review Best Practices
#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 think is important, is to not act as a blocker for code reviews unless its absolutely necessary. Lots of engineers take on the attitude that they're going to "gate" code they don't agree with by with holding their +1 and bogging down the review with questions and all sorts of runarounds till its what they want. this is a bad attitude to have, even when you're dealing with Junior engineers.
I'm generally going to +1 something unless I fundamentally disagree with it or think its going to break things in production. What I do, though, is leave lots of comments with questions/suggestions and mention it in the +1 with (see comments).
This builds trust on teams, and stops things getting personal, especially with people who aren't very good at dealing with criticism, even in something as banal as a CR. On a team that works well together, teammates will see those comments, think about them and make thoughtful responses, especially once they understand that you're not trying to get in their way. Giving the +1 gives them the freedom to consider your suggestions without being irritated that their PR is being blocked. They feel like they're in control not you.
In rare exceptions, someone will brush off my questions and merge ... which means that next time, I get to be tougher on the review and specifically ask for responses before the code can be merged, because they've degraded the implicit team trust. Usually repeat offenders are assholes, and assholes generally don't last on healthy teams.