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

81–90 of 376 posts

Re: How to do a code review

#81

Post by dxf »

aspaceman wrote: post #78
polishTar wrote: post #69

Earlier quoted context omitted.

> 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…

I agree - most folks overestimate the effectiveness of microbenchmarks. I work in a field where microbenchmarks often feel useless: computer graphics. Often we have to consider the entire pipeline of work. Writing to a texture in Stage 3 may seem perfectly fine, but it could be thrashing the cache in Stage 4 when the texture is being read from. Benchmarking them separately misses this.

You're right that many microbenchmarks can be noisy (and the environment you run them on can introduce more noise).

But if you're attending cppcon, two engineers in Google's production toolchain organization are giving a talk on "Releasing C++ Toolchains Weekly in a 'Live at Head' World" where they'll discuss (among other things) how we use various microbenchmarks to catch performance regressions.

https://sched.co/Sft4

Re: How to do a code review

#82

Post by civicsquid »

aviraldg wrote: post #13

This is great advice and isn't followed often enough, especially when reviewing code written by people new to an organization/team: > If you see something nice in the CL, tell the developer, especially when they addressed one of your comments in a great way. Code reviews often just focus on mistakes, but they should offer encouragement and appreciation for good practices, as well. It’s sometimes even more valuable, i…

Something I've started doing that I picked up was prefixing my nitpicky comments with "Nit: ..." so that it's clear that certain comments are just minor suggestions, not that anything is necessarily wrong.

I'm usually okay with preemptively accepting code with only nit comments too just to signal that those comments are not too big of an issue (if at all).

Re: How to do a code review

#84

Post by AnimalMuppet »

aledalgrande wrote: post #18

In the "What Is Not An Emergency?" section: > It is the end of the day on a Friday and it would just be great to get this CL in before the developer leaves for the weekend. I laughed out loud because it reminded me of so many times I have seen it happen and then someone had to fix in the weekend. Who shares the same experience?

Worse: The developer wants to get this checked in before they go on vacation.

And they really want to, because they don't want to have to do a bunch of merges when they come back...

Re: How to do a code review

#85

Post by YZF »

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…

The review should simply be gated on passing the automated bits (tests/lint/warnings/style checkers whatever).

Re: How to do a code review

#86

Post by luminiferous »

dymk wrote: post #64
Glyptodon wrote: post #27

Earlier quoted context omitted.

Some places I've worked explicitly won't push/deploy/similar on Friday.

But then Thursday just becomes Friday...

That's fine, because when something breaks the day after you've pushed, it's Friday and everyone's in the office. People are still going to rush the deadline, but putting the deadline earlier means you have time to deal with stuff that breaks after the deadline.

Re: How to do a code review

#87

Post by cameronbrown »

bt848 wrote: post #76
aledalgrande wrote: post #18

In the "What Is Not An Emergency?" section: > It is the end of the day on a Friday and it would just be great to get this CL in before the developer leaves for the weekend. I laughed out loud because it reminded me of so many times I have seen it happen and then someone had to fix in the weekend. Who shares the same experience?

They didn't give the two examples that are most common inside Google: Google I/O is coming up, or annual performance reviews are coming up. Strangely they also list as "not an emergency" rollbacks of clearly broken code, but that's an exception to review rules inside Google. Anyone can do a pure rollback of a change without getting the approval of the owners of the code, and there are automated tools that will roll b…

> at least 1000 tests

The threshold for this seems at least a little too high, doesn't it?

Re: How to do a code review

#88

Post by bt848 »

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 while, new people become acculturated and gain readability.

"Matter of judgement" is why hiring is absolutely critical to success in software. You must hire people with good judgement, and to the extent that you might intentional or unintentionally hire bozos, do so at a low enough rate that you can indoctrinate them.

Re: How to do a code review

#89

Post by wolrah »

d_burfoot wrote: post #59

Am I the only software engineer in the world who hates diff-based code review? Imagine trying to evaluate the quality of a novel by examining diffs to the manuscript. Reviewing every diff means that every single change needs to yield a good novel. But that's an absurd constraint on the creative process - what if you want to introduce a new important character? You check in a new version of the first chapter where the…

This is why a lot of projects use version control systems that encourage branching. Develop your feature as you wish in its own private branch, occasionally bringing in the latest changes from whatever your mainline branch may be, then only submit it for review when it's complete enough.

Re: How to do a code review

#90

Post by bt848 »

cameronbrown wrote: post #87
bt848 wrote: post #76

Earlier quoted context omitted.

They didn't give the two examples that are most common inside Google: Google I/O is coming up, or annual performance reviews are coming up. Strangely they also list as "not an emergency" rollbacks of clearly broken code, but that's an exception to review rules inside Google. Anyone can do a pure rollback of a change without getting the approval of the owners of the code, and there are automated tools that will roll b…

> at least 1000 tests The threshold for this seems at least a little too high, doesn't it?

They have a lot of tests :-) Abseil-cpp alone has 2400 tests, at least in the open-source tree.

https://github.com/abseil/abseil-cpp/tree/master/absl/base

Post reply on HN