When I receive a PR that has actual problems, I usually make a branch from the code in question, and submit a PR to that branch with tests highlighting the problem, or the beginnings of a refactor that I'm asking for. I often don't have time to fully flesh out the problems, but helping to write the software with the requestor makes for much faster and effective feedback than plain text comments.
How to do a code review
91–100 of 376 posts
Re: How to do a code review
#92Earlier quoted context omitted.
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.
People have their own internal stories about how good they are, and all that feedback can be very painful. It doesn't help that many comments are nitpicks and pedantry, made by people without much social empathy. This is especially true when sending a patch for a high-level review of your proof of concept, and the next thing you know people are complaining about your formatting.
It can be hard to review badly formatted code. The reviewer has to put in effort to understand your change, but they have to put in more effort to parse the change if the formatting is bad.
"Why is this function's return value not being checked? Oh, turns out this person used a yoda conditional even though the rest of the code doesn't do that."
"Why is this block of code running even when the condition is false? Oh, turns out the else branch already ended and this part of the code is just indented wrong."
And so on. Even something as minor as `foo ()` vs `foo()` can stand out and act as a constant stream of mental speedbumps.
Claiming your change is high-level / only looking for feedback to the overall design doesn't change that. You're asking for a code review because you want the reviewer to read your code, but reading it is exactly the part that they're finding hard to do.
And it's not excusable, but the reviewer might be insulted that the reviewee is wasting their time and their feedback may be ruder / snappier as a result. After all, the reviewee could easily have put in the effort to run the auto-formatter, follow the existing code's style, etc.
Re: How to do a code review
#93This 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 think one clear distinction is if the review makes you think (1) my code sucks or (2) these people despise me.
It's much harder to power through (2) than (1).
Re: How to do a code review
#94In the "What Is Not An Emergency?" section: > It is the end of the day on a Friday and it would just be great to get this CL in before the developer leaves for the weekend. I laughed out loud because it reminded me of so many times I have seen it happen and then someone had to fix in the weekend. Who shares the same experience?
Some places I've worked explicitly won't push/deploy/similar on Friday.
We had no-deploy Friday which made support & product people feel happy and devs dread the pileup on Monday. It has evolved into "don't deploy higher impact things on Friday" and "try not to deploy at all Friday afternoon". That gives us a half day to hear about issues and fix them before the weekend.
Re: How to do a code review
#95In the "What Is Not An Emergency?" section: > It is the end of the day on a Friday and it would just be great to get this CL in before the developer leaves for the weekend. I laughed out loud because it reminded me of so many times I have seen it happen and then someone had to fix in the weekend. Who shares the same experience?
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…
> Google I/O is coming up, or annual performance reviews are coming up.
As long as everyone admits it, sounds like those are actual emergencies :)
Re: How to do a code review
#96This 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.
Re: How to do a code review
#97If you were wondering what CL stands for since its not defined anywhere in this doc, it is apparently akin to a Pull Request.
Re: How to do a code review
#98> At Google, we hire great software engineers, and you are one of them. That's a pretty presumptuous comment.
Maybe so, but can you please not post unsubstantive comments to HN?
Re: How to do a code review
#99Here'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 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
#100I feel like the term "open source" is overused. For code, which is where the term came from in the first place, it's relatively easy to define; but what is "source" for something non-code? Is Google really wanting you to contribute to changing its internal processes? I'd say ..."are now public" makes more sense for releasing this document.
The submitted title was "Google's internal code review guidelines are now open source". We've since replaced that with the article title, in accordance with the HN guideline, "Please use the original title, unless it is misleading or linkbait; don't editorialize."