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

221–230 of 376 posts

Re: How to do a code review

#221

Post by sorokod »

de_watcher wrote: post #217
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.

Pretty much by definition you will be covering a wider surface of possible inputs and behaviours and so acquiring complexity

Re: How to do a code review

#222

Post by kortilla »

flowerlad wrote: post #133
jonahx wrote: post #123

Earlier quoted context omitted.

Preface: I was also very glad to see this called out specifically, and think it's a great rule. That said... > Note how Google does NOT say "make sure the code is properly architected". is not accurate. The very first paragraph on the same page is: > Design > The most important thing to cover in a review is the overall design of the CL. Do the interactions of various pieces of code in the CL make sense? Does this cha…

But that sure sounds like they care about proper architecture. Of course they care about architecture—I don’t think anyone implied otherwise. But at good companies like Google good architecture is a given. Top developers often fall into the pit of over-engineering, and almost never under-architect. So at top companies code reviewers have to be vigilant about over-engineering and rarely have to worry about under-archi…

>But at good companies like Google good architecture is a given.

Absolutely not. Google’s interview process and inflow of fresh graduates does not bode well for good architecture. Having spent time at G and FB, I can certainly tell you that employees at both are no better at architecting code in a sane way than SWEs at other companies.

Code architecture requires experience. Google does not.

Re: How to do a code review

#223

Post by jsolson »

amxpd wrote: post #96
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…

At Amazon, all code is reviewed. Comments are constructive and straight to the point. Standard courtesy applies, but anything more than that is left out. Unless there's something extremely interesting, overly cautious phrasing and "positive" comments are mostly noise. Also, people tend to specify when comments are nits.

> Unless there's something extremely interesting, overly cautious phrasing and "positive" comments are mostly noise.

Not really. Positive comments help teach engineers which things they've done that conform to local best practices (and why) without them having to meticulously dig those up (assuming they're even documented). A lack of positive comments leaves engineers to learn them only by running afoul of them. Effectively it provides direction only when some threshold of badness is crossed, while leaving positive comments on good code (especially for new team members or junior engineers) provides a beacon pointing away from the badness threshold entirely.

Put differently: commenting on good code makes for swifter and less eventful code reviews by steering engineers away from the bad practices that make code challenging to review in the first place.

Also, it's just nicer to spend forty hours a week with people who demonstrably appreciate their peers' good work.

Re: How to do a code review

#224

Post by username90 »

jorblumesea wrote: post #189
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…

That's kinda funny, because their interviews are exactly the opposite. Toy problems you'd never see in the real world, crazy abstracted solutions, O(n) demands, almost encyclopedic knowledge of data structures and algos. Do as I say, not as I do I guess.

Making code execute efficiently is not over-engineering, it is saving many millions of dollars and often a requirement to even launch.

Re: How to do a code review

#225

Post by Shaanie »

jorblumesea wrote: post #189
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…

That's kinda funny, because their interviews are exactly the opposite. Toy problems you'd never see in the real world, crazy abstracted solutions, O(n) demands, almost encyclopedic knowledge of data structures and algos. Do as I say, not as I do I guess.

I suppose you just wanted to complain, but in case you're serious: interviews and code reviews fulfill entirely different purposes. Choosing the right data structure may or may not be over-engineering, it depends on the application.

Google absolutely want developers that are capable of over-engineering.

Re: How to do a code review

#226

Post by kortilla »

iamchrisle wrote: post #216
kccqzy wrote: post #200

Earlier quoted context omitted.

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…

> I guess that's the nature of how code eventually matures over time.

Not really. It’s a sign of too many responsibilities being given to one project. Feature bloat is a real thing.

As code matures it shouldn’t be getting more and more abstraction and APIs, it should be stabilizing with fewer changes. If the former is happening, it’s a sign it needs to be broken into multiple projects.

Re: How to do a code review

#227

Post by kortilla »

Shaanie wrote: post #225
jorblumesea wrote: post #189

Earlier quoted context omitted.

That's kinda funny, because their interviews are exactly the opposite. Toy problems you'd never see in the real world, crazy abstracted solutions, O(n) demands, almost encyclopedic knowledge of data structures and algos. Do as I say, not as I do I guess.

I suppose you just wanted to complain, but in case you're serious: interviews and code reviews fulfill entirely different purposes. Choosing the right data structure may or may not be over-engineering, it depends on the application. Google absolutely want developers that are capable of over-engineering.

>are capable of over-engineering.

Anyone can over-engineer. It’s not a compliment nor a desirable quality.

Re: How to do a code review

#228

Post by kortilla »

aspaceman wrote: post #154
amxpd wrote: post #96

Earlier quoted context omitted.

At Amazon, all code is reviewed. Comments are constructive and straight to the point. Standard courtesy applies, but anything more than that is left out. Unless there's something extremely interesting, overly cautious phrasing and "positive" comments are mostly noise. Also, people tend to specify when comments are nits.

People call positive comments noise? They put scare quotes around “positive”? This field really scares me sometimes. Communicating positive feedback is crucial for teamwork, teaching, and passing ideas on.

Depends on level of seniority and amount of time the person has been on the team. Positive comments to a senior engineer about using a hash map instead of doing a huge linear scan are just noise.

Nurses don’t compliment each other for using a new pair of gloves on each patient.

Re: How to do a code review

#229

Post by mytailorisrich »

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…

Overall architecture should have been discussed and agreed before writing any code, not at code review.

Re: How to do a code review

#230

Post by tylerl »

A bit of useful flavor of how this looks in real life:

On my current team a typical code review of about 300 lines will involve about 3 to 4 reviewers, 20 comments, and maybe 10 changes, usually over the course of about 48 hours. Because this is a culture shock to people who join the company, we explicitly train every new team member ahead of time that code review suggestions are not a reflection on your code quality or your coding ability. Reviewers are expected to err on the side of raising a comment when in doubt, even (and especially) when the issue is about the approach being taken rather than the correctness of the code itself. Not every suggestion needs to be accepted, but every concern should be addressed. While not an explicit requirement, the expectation (and typical outcome) is that the committed code represents the combined expertise of the author and all the reviewers, and everyone is happy to go on the record as having been "responsible" for the code that got submitted.

A zero-question LGTM isn't an indication of good code, it's a red flag for an incomplete review.

Post reply on HN