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

181–190 of 376 posts

Re: How to do a code review

#181

Post by awinter-py »

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 codebase can be unrealistic -- sometimes there is no such person. Sometimes nobody is left in the company who understands the affected module and even the PR author is taking confession before merging it.

Love the phrase 'unless it's an emergency'. In resource-constrained cos everything is an emergency because to get any non-emergency work done requires emergency level urgency.

G's way of life requires an even power balance between eng & other silos. And also requires a baseline level of technical involvement by PMs which isn't present outside big tech cos. Most companies just aren't hiring PMs with a programming background. They could never justify the spend.

Major missing section in this is 'what shouldn't you review' -- various kinds of nits and out-of-scope changes that derail the process, waste expensive CI cycles, and make people take the rest of the day off from frustration.

Re: How to do a code review

#182

Post by onemoresoop »

Mvhsz wrote: post #163
ergothus wrote: post #155

Earlier quoted context omitted.

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…

And then you decide to get another job and next guy to maintain the code is utterly confused and misdirected because let’s be honest, there’s no documentation or there is but is either not up to date or sparse, theres no thorough unit testing (lucky if there is any). Not saying that’s your case but it happened to me personally to be on the other end a few times and let me tell you, it’s quite an effort to ramp up on such projects.

Re: How to do a code review

#183

Post by underwater »

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”

WIP PRs are fine. Essentially the PR is used as a mechanism to clearly communicate an idea, rather than containing code that is intended to be merged into the codebase.

There shouldn't really be a PR review where the author is seeking input on naming/testing completeness and major design choices. Those two types of reviews are mutually exclusive.

Alternatives to WIPs include whiteboard discussions, pair programming, and written proposals.

Re: How to do a code review

#184

Post by hankstenberg »

royalghost wrote: post #147

When a development team is following XP Principles, mostly two developers are doing Pair Programming with Test Driven Development (TDD) and rotating pairs every week, how much value will it add with code review ? We recently had a discussion where some of us think it is still good to skim through the change request by a second pair while other's think it's just a waste of time. I am curious to know if code review is…

Good point. At our company, we try to always assign two devs to more complex tasks, the actual developer and a "feature buddy". They don't actually PP the entire thing, but they discuss the strategy in pairs before starting to code and clarify question along the way in pairs. When the feature is done, the buddy will already have a very good idea about the feature and already agree with the fundamental decisions that were made along the way.

Re: How to do a code review

#185

Post by MichaelMoser123 »

CalChris wrote: post #137
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…

This is for a code review which would be well after architecture and be the entirely wrong place to systematically question architecture.

# This is for a code review which would be well after architecture and be the entirely wrong place to systematically question architecture.

I think that is part of the scrum philosophy: management can question whatever it wants and whenever it wants to; if the timeframe explodes that that is the blame of the developer.

Re: How to do a code review

#186

Post by UK-Al05 »

royalghost wrote: post #147

When a development team is following XP Principles, mostly two developers are doing Pair Programming with Test Driven Development (TDD) and rotating pairs every week, how much value will it add with code review ? We recently had a discussion where some of us think it is still good to skim through the change request by a second pair while other's think it's just a waste of time. I am curious to know if code review is…

There's been various studies saying that pair programming is not as effective as code review.

It's easy to see why. Code review you can focus on some code for awhile, pair programming you have to move on with the pace.

Re: How to do a code review

#187

Post by dwd »

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…

For a proper discussion I'm surprised no one has mentioned this principle from Extreme Programming possibly predates Google as a company.

https://ronjeffries.com/xprog/articles/practices/pracnotneed...

Re: How to do a code review

#188

Post by bruckie »

royalghost wrote: post #147

When a development team is following XP Principles, mostly two developers are doing Pair Programming with Test Driven Development (TDD) and rotating pairs every week, how much value will it add with code review ? We recently had a discussion where some of us think it is still good to skim through the change request by a second pair while other's think it's just a waste of time. I am curious to know if code review is…

At Google if person 1 and person 2 pair, then one can be the official author, and one can be the reviewer. Depending on the people and the change, they may also ask for review from others as well.

Re: How to do a code review

#189

Post by jorblumesea »

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.

Re: How to do a code review

#190

Post by deepaksurti »

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…

So for anyone, including me, when looking out for the next job should be:

- Is `good architecture` a given in this team?

- What are they doing to avoid `over engineering`?

What are the specific questions one can ask to find out the answers to the above two?

Post reply on HN