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

241–250 of 376 posts

Re: How to do a code review

#241

Post by hello_moto »

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…

Over-engineered is not a trap in BigCo, it is simply the by-product of KPI/Goal/Impact driven.

People tend to do it to get promo/bonus.

Re: How to do a code review

#242

Post by NKCSS »

Here's my workflow and it works very good to get everyone on the same level:

- Each night, I go over all the commits of the day and do a code review - Each morning at 9:00, we go over all the comments on the commints, with everyone, talk about it and make sure everyone understands.

This allows me to explain more advanced or new concepts that one developer uses to make sure everyone understands. It allows me to introduce improvements and explain why they are beneficial, and it allows me to signal when people don't follow the standards we impose; giving not only the person who did it wrong a refresher, but it makes sure everyone is reminded of the way we should write code, etc.

The feedback has been amazing and everyone loves it because it helps everyone grow and they feel like they are allowed to focus on quality; due to time pressures, coders tend to let standards slip when under pressure. When you focus on quality every day, it stays with them, because they know, if they don't deliver quality, it will be in the daily review. So far, this has not hurt production speed at all and quality has gone up a lot since we started doing this.

Re: How to do a code review

#243

Post by username90 »

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)

20% time still exists, you don't even need to tell your manager you are doing it and your manager has limited power to stop you from doing it if he learns about it. If he is trying to stop you then you can escalate.

Of course not a lot of people participate in 20% projects since just doing your main job is simpler than trying to balance two jobs. I only used it in short bursts myself, for example testing working on a project with a new team before transferring. However I feel it lives on in spirit, in that your coworkers and manager at Google typically doesn't know what you do from day to day and nobody will scold you or even notice if you work on a different project you feel is important for a week. If they make a fuzz just tell them it was your accumulated 20% time from the past 5 weeks.

In short, 20% time means that 80% of the time you belong to your manager and 20% of the time you belong to yourself. You are still judged by 100% of your contributions, so if your the new framework you decided to write didn't turn out to be useful then it wont be of much help for perf. But if it became a huge hit then that 20% project might be more important than your main project.

Re: How to do a code review

#244

Post by Cthulhu_ »

polishTar wrote: post #69
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 reall…

I want to say that yes, if it's simple and straightforward enough or obvious then just do it - it should have been written like that in the first place.

BUT I think a more important consideration is readability and clarity. Make it work, make it pretty, make it fast - in that order. If there was anything I learned from a Go course some time ago (Ultimate Go iirc) is that speed is a natural result of readable code. Plus if your code is good and well structured it becomes trivial to benchmark, identify and resolve any performance issues.

Re: How to do a code review

#245

Post by Traubenfuchs »

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.

Re: How to do a code review

#247

Post by Cthulhu_ »

ridaj wrote: post #151

It's very interesting that so much of it is about the social aspects of engineering - a lot of it kind of reads like " don't be a jerk ". In CS engineering classes, where presumably we would learn to become great engineers, I don't recall learning about any of this, and instead I remember the emphasis being on technical knowledge and accomplishment. I'd probably have been a better engineer in my early career if I'd u…

Your comment resonates with me and a book I recently read - Coders by Clive Thompson. It too points out how the emphasis has been on pure code, pure logic, merit over individuals, code over opinions, and of course the hero developer who doesn't want anyone else to work on their code.

Which of course is not reality. I mean I get it, I too would, I think, love to be a hero developer and just immerse myself into code for weeks on end, but that's simply not the reality. I don't write code, I contribute to a customer-facing website / shop, and at the moment my time and energy would be better spent adding a date picker than to fantasize about rebuilding the back-end in Go.

Re: How to do a code review

#249

Post by Cthulhu_ »

joefkelley wrote: post #38
Glyptodon wrote: post #27

Earlier quoted context omitted.

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

I worked at one (horrible) place where we explicitly only deployed on Friday afternoon. The logic was we have fewer users over the weekend, so if something goes wrong, fewer people will notice.

Ugh, I know the feeling - one client I worked for would only do deploys a few times a year if that, and only at night. I mean we'd get extra pay or free time for doing night shifts, but really, it's not good practice.

Re: How to do a code review

#250

Post by Smotko »

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…

I have worked on a Google project as a contractor and I can confirm that over-engineering was our biggest problem. I was also partially to blame, but at the time I did not know better.
Post reply on HN