Live data from Hacker News

A study of Google's code review tooling (Critique)

engineercodex.substack.com

221–230 of 302 posts

Re: A study of Google's code review tooling (Critique)

#221

Earlier quoted context omitted.

Anyone who has ever come along in a code review and told me my code “isn’t up to standard” will inevitably start a very long game of attrition with me. Here’s the thing: there’s no such thing as “standard” software architecture (until someone writes a new kind of software architecture called “standard”). I’ve seen and used everything from MVC to DDD to TDD to MVVC to whatever React is to reactive to event-oriented to…

>Anyone who has ever come along in a code review and told me my code “isn’t up to standard” will inevitably start a very long game of attrition with me. standard is relative, and while I'm talking generically as someone on the internet with no context of your experience, a code reviewer shouldn't. Someone literally saying "the code is not up to standards" needs to clarify. For my generic take, take "standard" as "wha…

To be clear, I'm not talking about code style (easily objectively quantifiable) or bad code (also easily objectively quantifiable), but architecture/design. This can be things like which folder an interface belongs in, where to put a method in a parent/child relationship that touches both, when to use value objects vs. scalars, when to break functionality into another service/library, which methods are allowed to call which methods, which classes must be injected vs. instantiated, etc.

Re: A study of Google's code review tooling (Critique)

#222
post #172

Earlier quoted context omitted.

I don't know if it's the tool honestly. I've recently switched teams and I'm in the process of getting java readability. The whole experience so far has been much worse than getting my C++ readability. I even get "nit" comments on the code I haven't changed. I have had multiple comments where reviewer basically "preferred" one style over another. It's been still mostly helpful, but I've had my share of frustration. C…

Getting reviews from multiple people that disagree on style is definitely an org problem that sucks to be in the middle of. However, I'd say that getting nit comments on code that surrounds your changes, but that you didn't change, is still fair game. It's part of the leave your campsite cleaner than you found it. It depends on the culture though - if it's suggested in the manner of "since you're here, here's an oppo…

I try to avoid nits totally unrelated to the changes at hand, since on a subconscious level they may discourage people from even wanting to touch older/less loved files at all.

The critical exception being avoiding issues due to path dependence.

E.g while a change is "correct" is doing X poorly because of surrounding issue Y. So we should fix Y now instead of building atop it.

Re: A study of Google's code review tooling (Critique)

#223
post #180

Earlier quoted context omitted.

Every while/for loop is essentially "keep doing this until I tell you to stop". You can argue what the best way to "tell to stop" is, but that doesn't really change the concept. No one could even articulate under which conditions this could happen. Cosmic rays altering memory? That's the kind of stuff we're talking about here. For a PHP webapp. I don't recall the exact specifics but we spent a long time on that stupi…

I can prove, pretty trivially that for i in range(10): Will halt. Slightly less trivially, you can prove that while true: if x: break else: break Will halt. But once the if conditions aren't exhaustive, it becomes difficult, perhaps impossible, to know that you won't end up in a weird unhandled state. Preferring and even insisting upon code that is not only possible, but easy, to reason about is good. Or iow it shoul…

You can prove that a different construct from a different language can do a different thing because it's a different construct from a different language.

> it should be incumbent on you to prove the loop halts

This is a nice one-liner platitude, but also completely unworkable.

Re: A study of Google's code review tooling (Critique)

#224

Earlier quoted context omitted.

No, it's not and there is a ton of literature (even entire books) written on why it isn't the time to bring this up during a review. For starters, it results in a waste of time for literally everyone involved: 1. The person writing has to rewrite it (probably). 2. The reviewer could have sat down with the person before a single line of code was written. 3. Anyone else reviewing just wasted their time because it will…

>The reviewer could have sat down with the person before a single line of code was written. This is a bigger waste of time. If every change requires asking someone in person, that's an infinite source of distraction and kills productivity for that person. MOST code reviews don't end up in comments asking for big changes, and when they do the one-time cost on the author is much better to pay than the constant tax on y…

How is spending 5 minutes x TEAM_SIZE to go over what you're going to do more time than 1-2 days solving a problem, then another hour or two for the reviewer to write out why it's wrong, then another day or two rewriting everything from scratch?

Re: A study of Google's code review tooling (Critique)

#225

Earlier quoted context omitted.

Team conventions are not cargo culting. Change is bad unless it's great. Being able to look around a codebase and know how things work because similar coding styles and patterns are consistently followed is a huge productivity boost (this is also what commenters complaining about the idea of readability elsewhere in this thread are missing). Something must be 10x better to compensate for diverging from those patterns…

Heh, if that's your reasoning on why something should be the way it is, then that is what it is. Somewhat reasonable, but don't be surprised if any reasonable person quits that day. The argument is not grounded at all in computer science or anything else objectionable. It doesn't allow the team to grow and change what is in front of them every day and forces them to live with old mistakes forever. Doesn't sound like…

I've worked with some amazing engineers, including multiple whose blog posts regularly get posted here. None of them have egos or consider code review feedback personal attacks.

It's not like you're being told to never use for loops. Code review feedback such as the following is all objective, beneficial to the reader, and not worth pushing back against:

* This logic should live in [other component] * Our RPCs are named as GetFoo, not DetermineFoo * That function name does not make it obvious what this code does, please change it * This needs a test * This is untestable and needs to be refactored to support X

If anyone on my team ever described themselves as in "a war of attrition" with another teammate I'd fire them.

Re: A study of Google's code review tooling (Critique)

#226

> Developers need to make progress; overly difficult reviews can discourage future improvements. This, to me, is the most important aspect of code review: get out of the way . If you are putting in nit picks, asking for changes that aren't related to a bug or bad architecture choice, etc, then get out of the way. If you are holding up the approval because you want someone to rewrite the code the way you would have wr…

Missing or misleading documentation. Confusing names. Lacking test coverage for new functionality. Unnecessarily complex code that can readily be simplified. Code that looks important but in fact does nothing useful. Unrelated changes mixed in with a patch that's ostensibly about something else.

These are all things I think are valid to point out in a code review. I don't think I would like to work in a company that rejects the notion.

Re: A study of Google's code review tooling (Critique)

#227

Earlier quoted context omitted.

I'd like to agree. But there is one part of that distinction that is real: there is such a thing as low quality code.

I think there is a such thing as “bad code” but not low quality code. Bad code can be detected by automated tooling and be improved through simple refactoring.

I'd say it this way: bad code doesn't robustly handle all the use cases, mis-interprets inputs sometimes, or is a mess to read and debug.

Yes, it matters what the code looks like. Knew a guy, named everything in his code a letter of the alphabet. a,b,c and when he got to z started za, zb etc.

That was 'bad code'. Or, it was 'Low Quality Code'. Nobody wants junk like that. Even if it passes tests, is robust etc.

Re: A study of Google's code review tooling (Critique)

#228
post #172

Earlier quoted context omitted.

I don't know if it's the tool honestly. I've recently switched teams and I'm in the process of getting java readability. The whole experience so far has been much worse than getting my C++ readability. I even get "nit" comments on the code I haven't changed. I have had multiple comments where reviewer basically "preferred" one style over another. It's been still mostly helpful, but I've had my share of frustration. C…

Getting reviews from multiple people that disagree on style is definitely an org problem that sucks to be in the middle of. However, I'd say that getting nit comments on code that surrounds your changes, but that you didn't change, is still fair game. It's part of the leave your campsite cleaner than you found it. It depends on the culture though - if it's suggested in the manner of "since you're here, here's an oppo…

Something I find helps with this in particular is only allowing style comments with citations to an actual style guide item. (I talk about domain-specific style guides as "crystallized arguments" - we agreed on this and wrote it down, not because it's necessarily right (though it probably is) but that we really wanted to stop wasting time arguing about these particular things.)

Re: A study of Google's code review tooling (Critique)

#229

Earlier quoted context omitted.

>The reviewer could have sat down with the person before a single line of code was written. This is a bigger waste of time. If every change requires asking someone in person, that's an infinite source of distraction and kills productivity for that person. MOST code reviews don't end up in comments asking for big changes, and when they do the one-time cost on the author is much better to pay than the constant tax on y…

How is spending 5 minutes x TEAM_SIZE to go over what you're going to do more time than 1-2 days solving a problem, then another hour or two for the reviewer to write out why it's wrong, then another day or two rewriting everything from scratch?

None of those timelines are realistic:

* Most CLs don't take multiple days to write

* Reasonably-sized CLs take 5-10 minutes to review - if I have things to say. Much less if the code looks good and I don't have any requests.

* I cannot think of any CL in the last 5 years where I made a comment that caused someone to "rewrite everything from scratch". Most feedback can be done in a few clicks in an IDE which supports refactor or a few minutes of copy/paste.

Re: A study of Google's code review tooling (Critique)

#230
post #54

Earlier quoted context omitted.

The fun thing to do in these situations is to add yourself as reviewer to all PRs by the person giving such feedback and return the favor. They learn pretty fast.

That seems like it risks creating conflict out of what's often just a misunderstanding. Assuming it's a corporate environment (it's fuzzier in the open source bazaar): If it's the first time or I don't really know the reviewer, I ask them to hop on a call to discuss (usually to walk me through) their feedback and I go in with an open mind. That gives me the opportunity to find out if I'm missing some context, can see…

If it's the first time - yeah, reach out to the person and talk to them.

But if the person is consistently leaving such comments under the guise of mentorship, 'raising the bar', or some other bullshit which boils down to them attempting to demonstrate their own seniority at the expense of other people's time and stress - then showing them how it feels is a great approach.

Bringing in other people and managers is not very effective I've found - it takes additional time, other people have their own stuff to focus on, and managers often don't have the technical expertise or confidence to push back against subjective comments which claim to be 'raising the bar' or whatever. It also doesn't look great when you have to bring other people in to help you address PR comments.

And, of course, you do this without ostensibly creating any conflict - if they complain simply respond along the lines of 'I totally love the care and attention to detail you bring when reviewing my PRs, I've learned from you and thought it would be appropriate to keep the same high standards and not lower the bar... etc'.

> As they go, if it isn't clear, I just ask them if something is a soft opinion or hard opinion.

Nah, if they don't explicitly state that's a soft opinion via 'nit', approving with comment or some other means, they are disrespecting the person who's PR they are reviewing. I shouldn't have to chase them down to see how strong their opinions are.

> If it keeps being in issue, then some frank conversations need to happen. Something I've noticed about folks who steadfastly focus on minor stylistic nits in CRs is they (1) tend to be cargo culting them without understanding the why behind them and (2) they're usually missing the forest (actual bugs in logic) for the trees.

What do you do if the comments are purely subjective and all backed up by internal/corporate dogmaspeak? 'Raising the bar' ... 'keeping the standards high' ... 'mentoring', etc. , or open ended comments asking to explain how stuff works, and whether 'this approach is the best'? There is no shortage of rhetorical bullshit that can be used to justify subjective PR comments.

> Most people are pretty reasonable when they don't feel like they're under attack, so in my experience it's usually possible to resolve these things without dragging it out.

The above will generally not work in a company that emphasizes PR comment count as a good metric for promotions/performance, and has a lot of internal rhetorical dogma. You WILL get people who leave these types of comments because they view it as a way of promoting their career, these people often cannot be reasoned with logically because they aren't actually all that smart, and they view any pushback against their comments as an attack against them.

Other people's feedback against the bullshit comments definitely help, but can look bad if you keep reaching out to other people to help address PR comments - I made sure to go through other people's PRs, on my own initiative, and refute bullshit comments when I realized how some people were behaving.

And yeah it totally depends on the company/team.

Post reply on HN