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

161–170 of 376 posts

Re: How to do a code review

#161

Post by BeetleB »

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…

>It's very interesting that so much of it is about the social aspects of engineering

Code reviews are mostly in the social domain and not the technical domain. Other than bugs and performance[1], everything in a typical code review is an opinion. Hence, you need guidelines on how to handle differences in opinions. Different work environments structure code reviews differently, but often the worst is where a reviewer finds problems, and the code review cannot be marked complete until the reviewer is satisfied. This often leads to "I have to change my code to the reviewer's preferred style" instead of "The reviewer found real issues". Other models, exist and are better.

[1] Actually, performance conversations are very often based on opinions. Things like what level of performance is acceptable, etc.

Re: How to do a code review

#162

Post by bt848 »

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

Sorry. I was responding mostly for this audience to the section about handling pushback. One common form of pushback I experienced was from authors who wanted an exception on performance grounds. These people were almost universally mislead, so I honed a specialized skill of disabusing wayward C++ authors of naive beliefs about performance.

Re: How to do a code review

#163

Post by Mvhsz »

ergothus wrote: post #155
underwater wrote: post #148

Earlier quoted context omitted.

I've been pushing the idea that if you're getting meaningful feedback on design (and over design) in your PR reviews than you've failed. That stuff should be shaken out long before you have working, complete code.

Is that a productive idea to push? Not to say you're wrong, but some hard human problems to solve are: - getting people to not be defensive during code reviews - getting people willing to be critical (constructively) of their peers work Emphasizing that code reviews with productive design comments indicates a failing seems more likely to stop the comments, not to improve the design. Most people wont want to do the wo…

There are times that I get into the flow while coding and end up solving a few future problems in addition to right now problems. I know that the code is over-engineered, but if it passes all of the tests and there's nothing obviously wrong with it I would probably check it in anyway. Over engineering never comes up in our code reviews but I think it would be a productive conversation if it did - even if the decision was still to accept the code as-is.

The programmers I work with can be defensive of their architecture, but they also love to talk about architecture generally. I think there's a "yes and" [0] way to bring this up that engages the whole team.

[0] https://en.m.wikipedia.org/wiki/Yes,_and...

Re: How to do a code review

#164

Post by jxramos »

jonahx wrote: post #123
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…

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…

what does the _CL_ notation indicate?

Re: How to do a code review

#165

Post by scruple »

jxramos wrote: post #164
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…

what does the _CL_ notation indicate?

Changelist, perhaps?

Re: How to do a code review

#166

Post by winrid »

Gibbon1 wrote: post #36
Glyptodon wrote: post #27

Earlier quoted context omitted.

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

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

Re: How to do a code review

#167

Post by Iv »

That advice is what I try to do more. I think people who veer more into the logic/engineering side of it forget some obvious emotional common sense:

> 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, in terms of mentoring, to tell a developer what they did right than to tell them what they did wrong.

Re: How to do a code review

#168

Post by pjm331 »

trgn wrote: post #152
underwater wrote: post #148

Earlier quoted context omitted.

I've been pushing the idea that if you're getting meaningful feedback on design (and over design) in your PR reviews than you've failed. That stuff should be shaken out long before you have working, complete code.

Yes! Engineers should collaborate on the overall approach to a code change/addition with the reviewers before any PR is submitted. Those early discussions are extremely important since teams will converge to an appropriate solution quicker because the process is much more informal. These frequent early discussions also build team spirit, and if you're so lucky, individuals will start to click and the team will start…

Out of curiosity how do those conversations happen before the PR?

Asking because we might have a different process for PRs or a different definition of what it means to submit a PR - on many teams I’ve been on it’s been encouraged to publish a WIP PR in order to facilitate these conversations. GitHub recently added a feature to formalize that but before this we used labels such as “WIP” and “Ready for review”

Re: How to do a code review

#169

Post by daxelrod »

civicsquid wrote: post #82
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…

Something I've started doing that I picked up was prefixing my nitpicky comments with "Nit: ..." so that it's clear that certain comments are just minor suggestions, not that anything is necessarily wrong. I'm usually okay with preemptively accepting code with only nit comments too just to signal that those comments are not too big of an issue (if at all).

My favorite way to do that has been to indicate “Not a blocker” to differentiate nits that I’m picking from things that would make me not approve the review.

Re: How to do a code review

#170

Post by trgn »

pjm331 wrote: post #168
trgn wrote: post #152

Earlier quoted context omitted.

Yes! Engineers should collaborate on the overall approach to a code change/addition with the reviewers before any PR is submitted. Those early discussions are extremely important since teams will converge to an appropriate solution quicker because the process is much more informal. These frequent early discussions also build team spirit, and if you're so lucky, individuals will start to click and the team will start…

Out of curiosity how do those conversations happen before the PR? Asking because we might have a different process for PRs or a different definition of what it means to submit a PR - on many teams I’ve been on it’s been encouraged to publish a WIP PR in order to facilitate these conversations. GitHub recently added a feature to formalize that but before this we used labels such as “WIP” and “Ready for review”

In person, or for a remote job, with screensharing and online chats. In my current job, it generally starts with the implementer reaching out and saying, I want to achieve xyz and think about doing abc. That's how the conversation gets started. Granted, sometimes you need code examples already ready-made to gather meaningful feedback, but that can be personal branch, one-off patch, gist, WIP PR ... feedback can happen over slack, simple chat, comments on the WIP PR ... As long as the final ready-for-review PR doesn't catch the reviewers off guard in terms of overal design/approach.
Post reply on HN