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

211–220 of 376 posts

Re: How to do a code review

#211

Post by cr1222 »

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…

Including a regression test is an excellent heuristic. I also like to extend that to testing closely related things where there are test gaps. The benefit is not just that it will catch the exact same bug recurring, but that you get more tests in areas of the code with higher bug density and more test gaps.

It's not a perfect heuristic but it's an easy one to get a team aligned on. IMO the biggest challenge in scaling a team and maintaining code health is setting standards like this that are easy to explain and easy to maintain. There's an unfortunate human tendency to cut corners in the absence of clear rules here (even the best engineers will do this in many circumstances).

You can always exercise judgement on the cost/benefit of adding the test though, it's just a good default.

Re: How to do a code review

#212

Post by cdcarter »

foobarian wrote: post #62
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…

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

I've been enjoying lots of comments in this thread, but unequivocally THIS. My company (for no particular reason) has two code review tools. Each team picks which one they like. Early in development I'll shelve a CL into the tool my team doesn't main, and just review and annotate diffs. It's a good place to keep TODOs and notes to myself.

Additionally, if I share it with my team, its clear that its "pre-review" quality and not to nitpick. "WIP" in the subject line helps, but being in an entirely different application really solidifies the point.

Then, when ready to go into real review, I've got a really good sense of how the CL looks as a diff, and where any remaining annotations need to be placed (that aren't truly a comment).

Re: How to do a code review

#213

Post by pjm331 »

ScottBurson wrote: post #191
pjm331 wrote: post #139

Earlier quoted context omitted.

I think of it this way: How can I build this so that it only solves today’s problems but doesn’t make it overly difficult to solve tomorrow’s problems? Loose coupling, dependency injection, composition over inheritance, and similar techniques tend to be good answers to this question in my experience. In contrast, over engineering attempts to solve tomorrow’s problems before they arrive and, if they arrive differently…

Very well said! My own experience is that effort invested in removing restrictions and handling corner cases is generally well spent. It may not seem too onerous to have to remember that a particular function works only for nonempty inputs or when called after some other function, but in a large system where many operations have such restrictions, keeping track of them quickly overwhelms the capacity of human memory.…

Yeah absolutely. I think a big part of it is considering possible edge cases or failure scenarios and not necessarily solving them immediately but considering how your design might need to be changed in order to solve them. Many times I have found there’s a solution that requires little or no more work than a naive implementation but is far more robust to future changes.

To use inheritance as an example - if there’s a conceivable possibility that some future requirement might lead you to add the same functionality to other classes, you probably don’t want to have to deal with a long and convoluted inheritance chain when you could compose some set of functionality onto one class as easily as many classes.

Or in the case of dependencies, wrapping some third party mail sending library in your own generic API is barely more work than using the third party library directly and will pay dividends if you change mail providers in the future.

One very specific example that I worked on recently: In most cases there are one of X, but in a handful of cases there are many of X. My initial inclination was to branch off and handle the many case as an exception, but then I realized that you could eliminate that corner case entirely if you handled everything as a collection of X, even if in most cases that collection only has one item in it.

Re: How to do a code review

#214

Post by iamchrisle »

awinter-py wrote: post #181

in real companies that don't have G's resources, quality and velocity are tradeoffs, not things you can have both of god bless them for trying though in particular, the idea of having one or more reviewers doing multiple rounds in a day is pure fiction in strapped startups. Rejecting a code review for readability isn't always an option. Even getting a timely review from someone who understands this part of the codeba…

I think the saying is "Quality, speed, cost. Pick two?"

Re: How to do a code review

#215

Post by tylerl »

awinter-py wrote: post #181

in real companies that don't have G's resources, quality and velocity are tradeoffs, not things you can have both of god bless them for trying though in particular, the idea of having one or more reviewers doing multiple rounds in a day is pure fiction in strapped startups. Rejecting a code review for readability isn't always an option. Even getting a timely review from someone who understands this part of the codeba…

Having experienced in real life the benefits of this policy myself, I absolutely disagree. I would institute Google-style code reviews at any company I went to that was large enough to have teams of 3 people or more. This as much for efficiency as for quality. The development pace isn't set by the speed at which new lines of code get added, is the speed at which features can reach maturity, and in general more extensive code reviews can speed things up in the aggregate in addition to making things work better.

At Google, every single line of code has to be signed off by at least one other engineer _before_ it can be committed to the official codebase, regardless of team size. Additionally, unit tests are required, typically at the same code gets reviewed. This all makes code submission significantly slower, but product development significantly faster. Why? Here's a few reasons I've personally observed:

1: Bus factor. People leave the team all the time, ideally not for the reason the concept is named after. Having someone thoroughly review your code means there are at least 2 people on the team who completely understand every single line of code. That guarantee isn't absolute just because of timing and churn, but it's correct often enough to be the rule. This simple fact has saved me, personally, _days_ worth of stumbling around. It saves you from precisely the failure condition you described.

2: Fewer changes. Especially paired with the "small CLs" advice, this helps you really quickly correct course when people are headed down the wrong solution. As a result, you end up shipping a much higher percentage of the code that you actually write.

3: Fewer bugs. This is the biggest item of them all. Bugs are the quicksand of coding... the metric you hear quoted is that companies are expected to waste 75% of their development time on debugging. Reviewers easily catch enough mistakes to more than pay for their time investment right there. This is especially true for misfeatures where the code is "correct", it just correctly does the wrong thing. Those mistakes are hard to catch with tests (because the tests will be equally wrong), but easy to catch with humans.

Also, this is where a zero-tolerance policy on requiring unit tests really shines. I've been at this for many years, and I have had zero (zero!) bugs in the unit-tested code that I've committed. My tests have found some that would have been real awful brain-melters though. I'm on slow end of things; it takes me about 5x as long to write the tests for a given module than to write the module itself, and I'm still WAY ahead on time when I account my own debugging savings (I think the 75% number is low if you want to hit the zero bug mark). On my (30-person) team, we spend about 5% to 10% of our time dealing with bugs in our own code, and regularly hit the "feature requests only" mark in our queue. And the bugs we do get we can generally eliminate as a class by teaching better review and test practices.

Re: How to do a code review

#216

Post by iamchrisle »

kccqzy wrote: post #200
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…

Does Google really practice what they preach though? Just recently I was looking at Angular, Google's web frontend framework. Services, modules, directives, angular-specific markup. Coming from React, I find this grossly over-engineered.

Googler here.

In general, yes we do practice it. They are guidelines, not rules tho.

Is Angular grossly over-engineered? Depends on how you look at it.

Any code's complexity reflects the complexity of its use case and Angular, for example, needs to fit A LOT of use cases at Google. So yea, sometimes, I think it can be a big hammer for a small nail. In other cases, all that abstraction helps. I guess that's the nature of how code eventually matures over time.

Re: How to do a code review

#217

Post by de_watcher »

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…

Sometimes you can generalize the problem a bit to get a shorter solution.

Re: How to do a code review

#218

Post by Gibbon1 »

winrid wrote: post #166
Gibbon1 wrote: post #36

Earlier quoted context omitted.

I've mostly worked for small industrial light manufacturers. First job I worked at they were always late with shipping stuff. And always taking stuff down to the UPS office after hours. After watching this for a few years I got the shop manager to adopt a rule. If it's not ready to be boxed up before lunch, it's not shipping today. After they started enforcing that they got a lot more productive and started shipping…

Forgive me, I don't understand your last sentence. It sounds contradictory vs "they got a lot more productive".

What would happen is the shop would start thrashing with people switching from one task to another trying to hurry one unit through production. You eat the setup time for each production task for exactly one unit.

Re: How to do a code review

#219

Post by throwamay1241 »

ereyes01 wrote: post #47
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…

I once worked on a team that specialized in very long littanies of code review comments... but they were able to bake this into their culture in fundamental ways such that it ended up being one of the most positive experiences in my software engineering career. The basics of how they accomplished this was: - The obvious- no personal / destructive attacks or insults, no cussing, no comments on any person's abilities.…

I found that really informal comments cut less deep 'hey man this needs double checking, read up on $x and then reconsider this block' (gender aside) is far better received than 'This is missing fundamental concepts around $x read up on them then rewrite this block'

Re: How to do a code review

#220

Post by esolyt »

Helpful and interesting.

However, you can tell this was written by an engineer. The acronym CL is used about a hundred times. There is not a single page where the first usage of the acronym explains the full term.

Post reply on HN