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

121–130 of 376 posts

Re: How to do a code review

#121

Post by derekp7 »

mieseratte wrote: post #25
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…

Why is it that people feel so discouraged by loads of review, especially early on? I always had a good bit of imposter syndrome early on, but never considered quitting. I always assumed that you have a lot to learn, that it’s expected you’re going to suck at some level.

There are often multiple ways to make a make a skinny cat, with no particular way better in all respects than any other method. Programmers end up designing code based on their own mental model, held in their head at that time, and would not really be able to explain that model to someone else. That someone else in turn has a different mental model of what is the correct way. Both ways are correct, just one of them is not what the reviewer would have done.

When the reviewer communicates this to the coder, this often comes across as criticism of code that is in reality perfectly fine.

Re: How to do a code review

#122

Post by sanderjd »

hota_mazi wrote: post #21
jonny_eh wrote: post #11

Earlier quoted context omitted.

aka "Pull Request" in Github parlance.

aka "Merge Request" in Gitlab vernacular (which makes more sense than "pull request" in my opinion).

Yeah, merge request seems like the best terminology of the ones I've seen.

Re: How to do a code review

#123

Post by jonahx »

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 change belong in your codebase, or in a library? Does it integrate well with the rest of your system? Is now a good time to add this functionality?

Emphasis mine. But that sure sounds like they care about proper architecture. They just also care about avoiding over-engineering. They're not mutually exclusive.

Re: How to do a code review

#124

Post by ehsankia »

derekp7 wrote: post #116
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…

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…

It obviously depend on the language and your tooling too, but I find that it's generally much easier to refactor an under-designed code to add functionality, than trying to refactor a code that was over-designed but doesn't fit the spec anymore.

For example, in Python, there's no need to go for a class when a simple function does the job. And once you do need a class, it's fairly trivial to swap one for the other, especially in a good IDE.

Re: How to do a code review

#125

Post by molodec »

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 agree that "must" is too strict. If I replace linear search with binary search, or hashmap/hashset lookup I don't need to write a benchmark to prove it improves performance. There is math, logic, Big O analysis that allows to reason about performance and speed without microbenchmarks.

Re: How to do a code review

#126

Post by foobarian »

bt848 wrote: post #88
YZF wrote: post #80

> Some hardward manufacturers only ship new hardware once a year. A line snuck in through their code review. One big issue that I'm not really seeing discussed is that in many cases code reviews end up being very subjective. "hard to maintain", "not readable", "brittle", "needs more tests" are all super subjective. There is no magic tool that can tell you the future maintenance costs (vs. the current costs), whether…

You can draw bright lines about some of these things. If a change claims to fix some bug, a test must demonstrate that. If I patch just the test into HEAD and run it, it should fail. If this is not the case then the change "needs more tests". "Not readable" is why Google also has the "readability" process. A person without readability needs the pre-submit approval of someone with readability in that language. After a…

On judgment: Linus has a great way of looking at it as taste. There is a short segment in his TED talk with a nice example.

https://www.youtube.com/watch?v=qrYt4bbEUrU starting at 14:20

Re: How to do a code review

#127

Post by enneff »

d_burfoot wrote: post #59

Am I the only software engineer in the world who hates diff-based code review? Imagine trying to evaluate the quality of a novel by examining diffs to the manuscript. Reviewing every diff means that every single change needs to yield a good novel. But that's an absurd constraint on the creative process - what if you want to introduce a new important character? You check in a new version of the first chapter where the…

Computer programs are nothing like novels. Novels are intended to be read from start to finish, while the parts of a computer program can be appreciated in isolation; in fact, well-written software is comprehensible when read "in the small". Code review encourages people to build software composed from write small, reusable components. That's a huge benefit.

As the guidelines say, they goal is not to check in perfect code, but to move the project forward. Sometimes that means adding code with a TODO comment (and an accompanying bug number) to remove a hack or implement something that's missing, and that's fine.

The main benefit of code review is that you're sharing and documenting your development process. Your teammates, by reviewing your code, certify that they understand what you're doing and why. Every commit you make should be explicable and justifiable in the context of code review; if not, then why are you making the commit?

Re: How to do a code review

#128

Post by sanderjd »

cameronbrown wrote: post #87
bt848 wrote: post #76

Earlier quoted context omitted.

They didn't give the two examples that are most common inside Google: Google I/O is coming up, or annual performance reviews are coming up. Strangely they also list as "not an emergency" rollbacks of clearly broken code, but that's an exception to review rules inside Google. Anyone can do a pure rollback of a change without getting the approval of the owners of the code, and there are automated tools that will roll b…

> at least 1000 tests The threshold for this seems at least a little too high, doesn't it?

I was thinking low :)

Re: How to do a code review

#129

Post by flowerlad »

derekp7 wrote: post #116
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…

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 can tell you what I do: I allow the total cost to increase by no more than 10% for future proofing. It is all about controlling cost. It is not justifiable to spend more than 10% of the time for future proofing because you have no idea what the future is going to be. Of course if you do have some idea about requirements coming in the near future then it may be justifiable to spend more.
Post reply on HN