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

251–260 of 376 posts

Re: How to do a code review

#251

Post by Cthulhu_ »

stackzero wrote: post #40

>In general, reviewers should favor approving a CL once it is in a state where it definitely improves the overall code health of the system being worked on, even if the CL isn’t perfect. This is a great rule of thumb

Yeah, that was the first section I read and I raised an eyebrow, I hadn't thought of it like that yet. I can see how one would get paralyzed by a focus on perfection; I too have left idk, dozens of comments on a feature PR of a few hundred lines of code.

Mind you that was before Prettier, nowadays a lot of these niggly things are automatically identified and fixed in editors and a pre-commit hook.

Re: How to do a code review

#252

Post by Cthulhu_ »

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…

And yet. Technical debt costs you time and money over time. Fixing code six months down the line takes 5-10x as much effort than just fixing it while it's still fresh. Worst case scenarios is where you end up throwing away an application and rebuild it because the technical debt has made progress grind to a halt.

If there's nobody left who understands the code, or if there's hero coders who 'own' one section of the application, it's a huge risk to the company - which can, and has, bring a whole company down (there's a few stories on the daily wtf iirc).

A recurring thought with me and my projects after about six months is always "I wish we had done this properly the first time around"; in hindsight there was more than enough time, and it's costing a lot more time to fix it now.

Re: How to do a code review

#253

Post by dgellow »

kortilla wrote: post #226
iamchrisle wrote: post #216

Earlier quoted context omitted.

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.

> If the former is happening, it’s a sign it needs to be broken into multiple projects.

Breaking something in smaller projects also has its own associated costs.

Re: How to do a code review

#254

Post by mav3rick »

Traubenfuchs wrote: post #245

I have never not seen code reviews end up being a massive social/cultural pain point for most people involved. Most people hate getting their code reviewed in depth. In situations where there isn't just one lead dev who is responsible for reviews, this leads to "Merge Request symbiosis", where two people uncritically approve each others requests so they don't have to deal with the third.

Code reviews by senior people are massively helpful for junior devs. It's one of the perks of being in a good big company that very few startups can replicate.

Re: How to do a code review

#255

Post by mav3rick »

kortilla wrote: post #222
flowerlad wrote: post #133

Earlier quoted context omitted.

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.

Most teams at Google have senior devs that ensure this. The average level is way higher than other companies.

Re: How to do a code review

#256

Post by dgellow »

vonseel wrote: post #146
pitaj wrote: post #114

Earlier quoted context omitted.

GitHub features the same draft system you described.

I thought theirs was immediate, but I was referring mostly to Bitbucket Server's system from previous experience with that.

GitHub has both. You can add an immediate comment, or add it to a draft that you post as one unit.

Re: How to do a code review

#257

Post by mav3rick »

Rapzid wrote: post #138
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…

Those are an oddly specific and narrowly focused set of recommendations to add to a set of guidelines that are largely very general and apply to a broad range of circumstances..

No it's very practical advice that I use day to day as well. Basically check the interface first. Implementations can be fixed but contracts are harder to change when clients depend on it.

Re: How to do a code review

#258

Post by mav3rick »

dmitrygr wrote: post #210
kccqzy wrote: post #205

Earlier quoted context omitted.

No, the point is Google goes beyond the typical organization in that it provides 20% time which can, if you desire, be used to work on personal projects even with Google equipment. That's better than using after-work hours and your own resources.

20% time hasn't existed in 8 years at least (source: spent 7.5 of them working at Google, various teams)

It does exist. You didn't do it doesn't mean others aren't doing it right now.

Re: How to do a code review

#259

Post by merb »

OT: but does somebody know a good code review tool, that fits nicely into git? (Bonus points if it works containerized) Should work for 5 people.

Re: How to do a code review

#260

Post by dgellow »

siquick wrote: post #140
hckr1292 wrote: post #33

Earlier quoted context omitted.

Social empathy is a really important skill, especially when delivering feedback. However, the best way to improve as a junior developer is to welcome feedback, not panic. I've seen junior developers welcome feedback or run from it, and I think that has as much to do with the reviewer as the author of the code. On the flip side, I've learned almost as much from reviewing more senior engineer's code and just asking dum…

I'm sure we've all worked in places where a senior would not like it if a junior reviewed their code, regardless of the juniors intent. But yes, always a tonne to learn. IMO the first few code reviews should be done face to face so some rapport can be built. Receiving feedback from a 'human' is far easier to process than a faceless Github profile picture.

The face to face is definitely a good approach for first reviews. Also it’s worth starting with lower expectations and less critical feedbacks then increase over time. It takes time for people to feel comfortable receiving direct critics from persons they aren’t familiar to.
Post reply on HN