Live data from Hacker News

How to do a code review

google.github.io

191–200 of 376 posts

Re: How to do a code review

#191
post #139

Earlier quoted context omitted.

It sounds like walking a tight line between over engineering, and falling into technical debt. If you design code specifically solves the immediate need, that may need to be thrown away or extensively worked on / around when future needs come up. On the other hand, you can write code that solves future needs that never appear, and still fail to solve the actual needs that end up appearing. For me, I would rather put…

I think of it this way: How can I build this so that it only solves today’s problems but doesn’t make it overly difficult to solve tomorrow’s problems? Loose coupling, dependency injection, composition over inheritance, and similar techniques tend to be good answers to this question in my experience. In contrast, over engineering attempts to solve tomorrow’s problems before they arrive and, if they arrive differently…

Very well said!

My own experience is that effort invested in removing restrictions and handling corner cases is generally well spent. It may not seem too onerous to have to remember that a particular function works only for nonempty inputs or when called after some other function, but in a large system where many operations have such restrictions, keeping track of them quickly overwhelms the capacity of human memory. Sooner or later someone is bound to forget, and it may even be the author of the code in question. I try to ask "what are people going to expect this code to do?" and then, if within reason, to make it do that (and failing that, to protect it with assertions). Alternatively, someone may be forced to make some other part of the system more complicated to work around the restriction, leading to excessive coupling.

I suppose this practice sometimes risks over-engineering, but I have found the risk to be worth taking. As you say, it makes the system easier to extend further.

Re: How to do a code review

#192
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" is the term perforce uses for a a single commit before it gets committed. A bit like a PR in git. Google used perforce before rolling their own vcs, and they kept the perforce terminology.

Re: How to do a code review

#193
post #81

Earlier quoted context omitted.

I agree - most folks overestimate the effectiveness of microbenchmarks. I work in a field where microbenchmarks often feel useless: computer graphics. Often we have to consider the entire pipeline of work. Writing to a texture in Stage 3 may seem perfectly fine, but it could be thrashing the cache in Stage 4 when the texture is being read from. Benchmarking them separately misses this.

You're right that many microbenchmarks can be noisy (and the environment you run them on can introduce more noise). But if you're attending cppcon, two engineers in Google's production toolchain organization are giving a talk on "Releasing C++ Toolchains Weekly in a 'Live at Head' World" where they'll discuss (among other things) how we use various microbenchmarks to catch performance regressions. https://sched.co/Sf…

Ah I’ll take a look!

Re: How to do a code review

#194
post #168
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”

For smaller changes it can happen over a coffee in the hallway. For large changes there are design reviews. For stuff in between you can schedule a meeting with a few concerned people.

Re: How to do a code review

#195
post #111
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…

The quote, for people on mobile: > 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 might need…

Thanks!

I agree so much. Zero-cost abstraction is a misnomer.

Re: How to do a code review

#196
post #103
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…

If your making mistakes like over engineering are you actually a good developer then if that is part of what bad code is. Maybe you fall into the expert begginer at that point

It can happen when a really good coder gets bored.

Re: How to do a code review

#197

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…

Want to know a great way to handle a subset of these? Good automated code linters. Google has a ton of formatters and linters that either auto correct or suggest changes. To paraphrase a smarter Googler than me: People tend to just do what the bots say and don't care too much about. The best way to impose your code ideas on others is via a linter.

Automated tools here have also drastically reduced the number of comments I get on CLs at Google, as people don't bicker about the little things as much anymore.

Re: How to do a code review

#198

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.

Jeff Dean and Sanjay Ghemawat coded like this for years.

Re: How to do a code review

#199

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…

It's not supposed to be a one size fits all I guess.

For example, their recommended order of reviewing code tests first doesn't exactly work when you have no tests, as is pretty common at startups.

Re: How to do a code review

#200
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…

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.

Post reply on HN