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

111–120 of 376 posts

Re: How to do a code review

#111

Post by pitaj »

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…

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 to be solved in the future. The future problem should be solved once it arrives and you can see its actual shape and requirements in the physical universe.

Re: How to do a code review

#112

Post by vonseel »

siquick wrote: post #22
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…

> I've seen cases where people got hundreds of comments (many of them minor, nitpicky) from more experienced developers and were discouraged by the sheer number of them. That most new developers naturally suffer from imposter syndrome is not helped at all by 100% critical code reviews. This, combined with a large portion of developers lacking social empathy, poor communication skills, and (unfortunately) a desire to…

> hundreds of comments (many of them minor, nitpicky)

This is one reason I liked Phabricator’s review system, which allowed drafting comments on an entire PR before submitting the comments. This allows you to be as nit-picky as you want when reading the PR, and then delete or modify any of them at the end. Instant submission of line-level comments, on the other hand...

I think reviewers should have the awareness to understand not to over-burden the person whose code they are reviewing.

For example, if I’m reading some very junior code and finding lots of issues, maybe I should delete all my comments about minor things like style and only leave the comments requesting major fixes like code design or potential bugs? When the developer re-submits the code after fixing the main issues, maybe it will be less frustrating to get a review back with “this looks great, can you just fix these small issues that don’t conform to our style guide and it’s good to go.”?

Re: How to do a code review

#113

Post by enneff »

YZF wrote: post #102
bt848 wrote: post #88

Earlier quoted context omitted.

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…

"every bug fix must be accompanied with a test that demonstrates the bug/fix" is just dogma IMNSHO. (also see "closing the barn doors after the horses have escaped" or "lightning doesn't strike twice at the same place") The typical rationale is the tests will now catch a re-introduction of the bug or regression. The reality is that often the tests bloat the code base, they cost in future maintenance making the code h…

If the added test makes your code hard to refactor, then the test is too low-level. Trigger the bug by exercising the higher level abstractions, then refactoring won't care about the test (and in fact, you'll be glad the test is there when you refactor).

WRT "brittle", sometimes a test fails when you're doing maintenance; you can look at the test and sometimes it makes sense to delete it, sometimes it shows you that your maintenance has unintended consequences. You can't make that judgment call if you didn't write/keep the test.

Re: How to do a code review

#114

Post by pitaj »

vonseel wrote: post #112
siquick wrote: post #22

Earlier quoted context omitted.

> I've seen cases where people got hundreds of comments (many of them minor, nitpicky) from more experienced developers and were discouraged by the sheer number of them. That most new developers naturally suffer from imposter syndrome is not helped at all by 100% critical code reviews. This, combined with a large portion of developers lacking social empathy, poor communication skills, and (unfortunately) a desire to…

> hundreds of comments (many of them minor, nitpicky) This is one reason I liked Phabricator’s review system, which allowed drafting comments on an entire PR before submitting the comments. This allows you to be as nit-picky as you want when reading the PR, and then delete or modify any of them at the end. Instant submission of line-level comments, on the other hand... I think reviewers should have the awareness to u…

GitHub features the same draft system you described.

Re: How to do a code review

#116

Post by derekp7 »

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 a bit of additional effort up front gathering enough information about the direction of the project or usage of it to better formulate an attack plan so that it does actually become more future proof. But even that is subject to failure, so what do you do?

Re: How to do a code review

#117

Post by sanderjd »

amxpd wrote: post #96
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…

At Amazon, all code is reviewed. Comments are constructive and straight to the point. Standard courtesy applies, but anything more than that is left out. Unless there's something extremely interesting, overly cautious phrasing and "positive" comments are mostly noise. Also, people tend to specify when comments are nits.

Why the scare quotes around "positive"? I have lots of positive thoughts when doing code reviews. I don't write them into comments all that often, but sometimes I do. "Oh I didn't know about this API, nice find!" "Ah, nice approach, this is a big improvement." "Thanks for improving the test coverage!" Maybe this seems like unactionable "noise", but it isn't, it encourages future actions of the kind being positively reinforced. Plus it's free, it takes no time to write and no time to read comments like this, all it does is make the author's experience of reading the review a bit more pleasant. This is all upside, no downside. It would be crazy to discourage such a thing.

Re: How to do a code review

#118

Post by enneff »

electrograv wrote: post #20
whydoyoucare wrote: post #14

"Technical facts and data overrule opinions and personal preferences." So true, so true! Now if teams pay careful attention to this statement, the Code Review Guidelines can be shortened by an order of a magnitude! ;-)

As a general rule, absolutely! Unfortunately, if applied too strictly though, it can be weaponized or stifling in ways you may not anticipate (which tends to be true of almost any rule used too universally). For example: I’ve seen cases where there’s disagreement as to which of two technical paths to pursue, where one side (not liking the way consensus is currently leaning) demands a data driven proof that one of the…

> in this case, it would take no less work/time to gather said data than to entirely implement both solutions and directly compare them.

So why not do this?

Re: How to do a code review

#119

Post by kbrackbill »

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.

I never considered quitting, but it still feels bad. Putting your code out there for the first time feels vulnerable (sort of similar to publishing your writing or public speaking) and it feels bad to get shit on.

When you've been working for a while you can filter out the noise regarding formatting and other smaller issues (or, ideally, get autoformatting set up), and it gets easier to separate your ego from your code. When you're new though, and showing someone else your code for the first time it's hard to see a huge volume of what seems like substantial negative feedback on your code (and by extension yourself).

I realize the "right" solution here is that people shouldn't equate criticism of their code with criticism of themselves, but I think that's a huge thing to expect at first and contributes to making the field unwelcoming.

Post reply on HN