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

61–70 of 376 posts

Re: How to do a code review

#61

Post by stiglitz »

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…

“You might consciously check in some bad code temporarily, intending to upgrade the code at some future point.“

The problem is how often devs forget about “temporary” code.

Re: How to do a code review

#62

Post by foobarian »

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…

Re: performance claims, well put. I often see coding decisions and review claims made on untested or outdated facts. For Java specifically three most common things I run across are:

1) Decision to use some kind of 3rd party library that purportedly speeds up handling primitive types (Trove specifically. Almost never worth it, and it's extra painful because it doesn't plug in well into the rest of the standard library).

2) Using mutable objects to avoid the inefficiencies of allocation and copying with immutable patterns. Can lead to a brittle stateful design with hard to track down bugs.

3) Reusing serialization artifacts across the call stack to save copying/allocation, again like 2). Now you end up coupling the API interface to layers deep in the call stack making things harder to modify.

Also best review advice I got and follow: be the first reviewer yourself! Looking at a nicely formatted diff really lets you see your code in a new light, and usually I end up doing several iterations before I run out of things to tweak. Things like forgetting debug log statements or commented out code, or dev settings, as well as other easy but effective improvements.

Re: How to do a code review

#63

Post by ng12 »

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…

Because grokking 20 diffs across 5 files is a lot easier than grokking the entirety of those files and the context on which they live. If you assume the rest of the codebase meets some reasonable standard of quality the diff should be the only important thing.

That said, for particularly complicated changes I'll check out the code and read through it locally but diffing is surely optimized for the most common case.

Re: How to do a code review

#64

Post by dymk »

Glyptodon wrote: post #27
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?

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

But then Thursday just becomes Friday...

Re: How to do a code review

#65

Post by bt848 »

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…

There's no reason you can't incrementally develop your change over time and then send it when it looks right. You don't need review for every edit. At Google you only need review to commit your change to the mainline repository. For long-running projects it's also permitted at that company to check in whatever you'd like into //experimental/users//...

Re: How to do a code review

#66

Post by dymk »

userbinator wrote: post #58

I feel like the term "open source" is overused. For code, which is where the term came from in the first place, it's relatively easy to define; but what is "source" for something non-code? Is Google really wanting you to contribute to changing its internal processes? I'd say ..."are now public" makes more sense for releasing this document.

They're accepting pull requests: https://github.com/google/eng-practices/pulls?q=is%3Apr+is%3...

Re: How to do a code review

#67

Post by xtian »

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…

> The main goal of this diff-based code review seems to be to prevent people from checking in bad code.

Perhaps some people use code review that way but what is "bad code"? Code that doesn't match your style guide? Style checks are better applied by automated tools. If you mean code that isn't as polished as possible, then the solution is to agree upon expectations with the rest of your team appropriate to the project and/or task.

Better goals for code review are knowledge transfer (both ways), design review (ideally on an early WIP branch), and catching mistakes and oversights.

Re: How to do a code review

#68

Post by Ididntdothis »

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…

Agreed. You don’t need fake praise but you should know whether something you did is just adequate or really good. You rarely have discussions about why something is good.

Re: How to do a code review

#69

Post by polishTar »

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 really critical part of the code, then sure, write a benchmark. Outside of that though, it's rarely a good use of developer time.

Post reply on HN