Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

How to do a code review

google.github.io

Post reply on HN
Print view View original

101–110 of 376 posts

Re: How to do a code review

#101

Post by the_arun »

Arnavion wrote: post #92
compiler-guy wrote: post #32

Earlier quoted context omitted.

People have their own internal stories about how good they are, and all that feedback can be very painful. It doesn't help that many comments are nitpicks and pedantry, made by people without much social empathy. This is especially true when sending a patch for a high-level review of your proof of concept, and the next thing you know people are complaining about your formatting.

It can be hard to review badly formatted code. The reviewer has to put in effort to understand your change, but they have to put in more effort to parse the change if the formatting is bad. "Why is this function's return value not being checked? Oh, turns out this person used a yoda conditional even though the rest of the code doesn't do that." "Why is this block of code running even when the condition is false? Oh,…

The formatting & style could be automated via IDE configurations, so that when the code is submitted for review it is already clean. This will make reviewer to focus on the design/logic instead of formatting.

Re: How to do a code review

#102

Post by YZF »

bt848 wrote: post #88
YZF wrote: post #80

> Some hardward manufacturers only ship new hardware once a year. A line snuck in through their code review. One big issue that I'm not really seeing discussed is that in many cases code reviews end up being very subjective. "hard to maintain", "not readable", "brittle", "needs more tests" are all super subjective. There is no magic tool that can tell you the future maintenance costs (vs. the current costs), whether…

You can draw bright lines about some of these things. If a change claims to fix some bug, a test must demonstrate that. If I patch just the test into HEAD and run it, it should fail. If this is not the case then the change "needs more tests". "Not readable" is why Google also has the "readability" process. A person without readability needs the pre-submit approval of someone with readability in that language. After a…

"every bug fix must be accompanied with a test that demonstrates the bug/fix" is just dogma IMNSHO. (also see "closing the barn doors after the horses have escaped" or "lightning doesn't strike twice at the same place")

The typical rationale is the tests will now catch a re-introduction of the bug or regression. The reality is that often the tests bloat the code base, they cost in future maintenance making the code harder to refactor, they may introduce their own flakiness (the test itself might have bugs), and the bug that's fixed is generally unlikely to recur. You want good tests- not just a random collection of tests that reflect the history of bugs in the code. I often end up writing a test as part of trying to understand or reproduce the bug, and that test will often stay... But not always.

Sometimes it's the right thing to do. Sometimes it's not. I.e. subjective. The lines aren't always so bright.

To complicate things further it's very hard to gauge the effectiveness of various processes. Is following those specific guidelines a net positive or negative? who knows. I think there's consensus that code reviews in general are a good idea so probably just by having a second person read the code, have a discussion with the author, with no guidelines, you're getting most of the benefit (obvious issues surface, more than one person is familiar with the code etc.). My gut feel is that the rest is in the noise.

I do agree hiring is critical ;) yet another subjective process there. Ideally you'd only hire people with a track record of producing good software in your area of business, most other indicators are probably pretty random. If you're looking to build a first person shooter you should hire John Carmack. Hope he likes your code review guidelines though.

Another random side comment is that IMO design issues should probably be identified before code is written.

EDIT: random semi-related trivia: https://kotaku.com/the-exceptional-beauty-of-doom-3s-source-...

Re: How to do a code review

#103

Post by tayo42 »

petilon wrote: post #99

Here's the part that resonated with me most: A particular type of complexity is over-engineering, where developers have made the code more generic than it needs to be, or added functionality that isn’t presently needed by the system. Reviewers should be especially vigilant about over-engineering. Encourage developers to solve the problem they know needs to be solved now, not the problem that the developer speculates…

If your making mistakes like over engineering are you actually a good developer then if that is part of what bad code is. Maybe you fall into the expert begginer at that point

Re: How to do a code review

#104

Post by rammy1234 »

euske wrote: post #7

To me this doesn't look a very good guideline. It's needlessly verbose and unintuitive. Instead of writing a long sprawling list of things, just using bullet points and figures would be better. It mentions the importance of getting a big picture of the code in question first, but the guideline itself is pretty bad at presenting its big picture. Here's the gist of it as I understand: - Long-term health of the code is…

Guidelines this big is there for a reason , lot of minds have gone behind this and every thing is a read to resonate with and come up as you understand it. Bulleted points and gist are good , but we are here to revise what we studied last night. This is a senior developer talking to a junior developer who is starting out. Also this is something go back and discuss and enhance.

Re: How to do a code review

#105

Post by atsaloli »

mattsfrey wrote: post #2

Would help if they mention what a "CL" is somewhere before using it ubiquitously

It's defined on the front page, " rel="nofollow">https://google.github.io/eng-practices/>, under Terminology:

CL: Stands for “changelist,” which means one self-contained change that has been submitted to version control or which is undergoing code review. Other organizations often call this a “change” or a “patch.”

Re: How to do a code review

#106

Post by compiler-guy »

Arnavion wrote: post #92
compiler-guy wrote: post #32

Earlier quoted context omitted.

People have their own internal stories about how good they are, and all that feedback can be very painful. It doesn't help that many comments are nitpicks and pedantry, made by people without much social empathy. This is especially true when sending a patch for a high-level review of your proof of concept, and the next thing you know people are complaining about your formatting.

It can be hard to review badly formatted code. The reviewer has to put in effort to understand your change, but they have to put in more effort to parse the change if the formatting is bad. "Why is this function's return value not being checked? Oh, turns out this person used a yoda conditional even though the rest of the code doesn't do that." "Why is this block of code running even when the condition is false? Oh,…

Prose editors have various levels of edits, edits for content, edits for organization, and copy editing and finally proofreading.

Yes, one should avoid misspelling words when you are about to submit a manuscript for content, but wordsmithing all the the sentences is a mistake.

Likewise, one should run an auto formatter before sending a proof of concept off for review, but a reviewer who is all tied in knots about low-level nits when looking at a general proof of concept is editing at the wrong level.

Otherwise you end up with a fully-baked, carefully written solution that satisfies all the nits, but was a giant waste of time because it takes the wrong overall approach.

Re: How to do a code review

#107

Post by bt848 »

YZF wrote: post #102
bt848 wrote: post #88

Earlier quoted context omitted.

You can draw bright lines about some of these things. If a change claims to fix some bug, a test must demonstrate that. If I patch just the test into HEAD and run it, it should fail. If this is not the case then the change "needs more tests". "Not readable" is why Google also has the "readability" process. A person without readability needs the pre-submit approval of someone with readability in that language. After a…

"every bug fix must be accompanied with a test that demonstrates the bug/fix" is just dogma IMNSHO. (also see "closing the barn doors after the horses have escaped" or "lightning doesn't strike twice at the same place") The typical rationale is the tests will now catch a re-introduction of the bug or regression. The reality is that often the tests bloat the code base, they cost in future maintenance making the code h…

https://mobile.twitter.com/dvyukov/status/116954416787113165...

This tweet perfectly captures why all bug fixes must have tests. Linux has no tests and no testing culture and it is 17 million lines of juicy hot garbage. Google is mostly tests and it is one of the largest and most successful C++ projects in history of our industry. I feel justified in standing my ground when reviewing under-tested code.

Re: How to do a code review

#108

Post by nimblegorilla »

polishTar wrote: post #69
bt848 wrote: post #53

I'd add two things, from a decade of experience at Google: Review code in this order: protocol buffers, unit tests, headers, implementation. It's common for a new employee to be an expert on C++ or Java or whatever languages but it's very uncommon to meet anyone who knows how to define a decent protocol message. The tests should give the reviewer a very clear idea of what's happening in the code (and this should be c…

> Any claims about speed, efficiency, or performance whether in comments or during the review must have accompanying microbenchmarks or they should be deleted. That's way too strict. If I want to suggest moving a statement that's inside a loop to the outside, I don't want to waste my time writing microbenchmarks showing that it improves performance. If the suggestion is complicated or non-obvious, and it's in a reall…

> If the suggestion is complicated or non-obvious, and it's in a really critical part of the code, then sure, write a benchmark. Outside of that though, it's rarely a good use of developer time.

If it isn't worth your time to write a benchmark then it isn't worth my time to change the code.

Everyone thinks their own performance tweaks are simple and obvious, but I've rarely seen measurable performance boosts from comments in a typical PR.

Re: How to do a code review

#109

Post by compiler-guy »

the_arun wrote: post #101
Arnavion wrote: post #92

Earlier quoted context omitted.

It can be hard to review badly formatted code. The reviewer has to put in effort to understand your change, but they have to put in more effort to parse the change if the formatting is bad. "Why is this function's return value not being checked? Oh, turns out this person used a yoda conditional even though the rest of the code doesn't do that." "Why is this block of code running even when the condition is false? Oh,…

The formatting & style could be automated via IDE configurations, so that when the code is submitted for review it is already clean. This will make reviewer to focus on the design/logic instead of formatting.

Autoformatters fix many things, but not all of these issues.
Post reply on HN