Live data from Hacker News

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

engineercodex.substack.com

211–220 of 302 posts

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

#211

Earlier quoted context omitted.

1. That's the point. It's not up to standard 2. They could but they usually don't. It's fine to sit down and make sure they understand some esoteric structure within a system I owj I'm not going to sit down with someone else to make sure they use for each loops unless it's a very new junior (i wouldn't block that in most code reviews anyway unless the code was legitimately painful to read, but every company is differ…

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 "whatever that team has laid out in its code architecture". Sometimes there may be times to question the standards if it compromises other important factors, but personally I don't think the code review is the time to fight over whitespace and bracket placement (those should be solved via a linter anyway). Make the correction and submit, those later discussions can happen offline.

>A code review simply isn’t the place to enforce your unique perspectives on someone else.

Likewise, the goal is to align everybody, not to argue over semantics or enforce your own POV. That's why code style sheets are a thing; it's a mediator that can serve as its own battlground should team/company style need to be changed. Again, every place is different, but

1. style should be automated as much as possible. If there's some stupid whitespace rule, it should be a one button click on my IDE from some provided linter to resolve (even if I will proceed to re-lint it to my style afterwards on my local machine).

2. style sheets can have suggestions as well as laws. Be reasonable on what is what

>For some people, they want layers.

sure, lawful evils exist, both maliciously and inadvertently. Sounds like you ran into both kinds. Office politics are inescapable even at the best companies.

That sounds like another discussion to adjust to style sheet, not throw it out upright. If you see a bunch of examples of a law being broken, adjust it to a suggestion unless is a strong argument made otherwise. (most) companies aren't a congress where amending such things takes months of proposal, and we can very much adjust the document to the people if there's no resistance.

Now, do you actually care enough to spearhead that change? I imagine many don't, and there in lies the problem. That's why I'm not on management track; I don't have the care nor attention to worry about documenting such things unless someone throws it at me. I'll leave that to those who do care.

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

#212

Earlier quoted context omitted.

This kind of greedy optimization approach is fine if all you’re just trying to maximize velocity, but it starts showing cracks on a long enough timeline in sufficiently large or complex projects. Speed without alignment on direction leads to ugly, tangled messes. As with most quality issues, the key is to try and surface quality problems as early as possible. Finding a bug in prod is worse than finding it in PR is wo…

> it starts showing cracks on a long enough timeline in sufficiently large or complex projects Show me a project which this doesn't happen to on a long timeline and I'll show you a project with no users and perfect programmers. Entropy is unavoidable. It will eventually degrade despite your best efforts and in the meantime you aren't benefiting from a working feature. We should try to delay software entropy as much a…

Read the Redis code base please.

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

#213
post #98

Earlier quoted context omitted.

> A PR is waaaaay too late to bring up No. That’s literally the point of a review.

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 your subject matter experts of what you're proposing.

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

#214

Earlier quoted context omitted.

My response would be a simple "Why does code changing frequently prevent inheritance from being used?" if I got a comment like that. Granted, I don't like inheritance, so I have nearly 1000 arguments on reasons not to use it that has nothing to do with code changing frequently, so ... this is probably a bad example for me, personally. But seriously, I'd ask why, and why again. I'm very much against cargo culting, and…

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 a good place to work. You don't get to a 10x solution overnight, in a single PR, you get there in increments.

I also disagree with it being "the team's code":

What you write is your code (and copyright law almost universally backs this up), what gets merged is a maintenance burden for all time. It very much matters what you like and don't like, and it very much matters that it is maintainable (whatever that means).

The team ... doesn't matter when it comes to code conventions ... they'll all be gone and moved on to other parts of the code/company/industry outside of five years. Most code lives long beyond today's team.

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

#215

Earlier quoted context omitted.

Excessive subjective feedback can be soul-crushing.

Anything subjective that doesn't fix an identifiable execution problem must be explicitly labeled as a suggestion . You don't get first choice over the code because you are asked to be the reviewer. You are there to (1) catch mistakes and (2) teach the other coder if they appear to not know something useful that you do know. If you have a preference about a simple stylistic matter that is not covered by your style gu…

The only exception that I would tag on to that is documentation. "I can't understand this documentation/comment" deserves more attention than I think most places give it.

It doesn't directly relate to execution today, but it might later on when someone misunderstands the docs or just gives up on them and tries to hack it together blind.

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

#217
post #152

I don't see it mentioned, but there's a recent addition that greatly improved my turnaround time to review incoming CLs: A prompt will show up in the corner about the next pending CL that you can approve, when you give LGTM to the CL that you are reviewing. It's kind of like those "Customers who bought this also bought" prompts on e-commerce websites. Only here it slightly nudges you to have a "CL review streak" and…

I never thought about this but it’s a fantastic idea. Would love to have something like this for GitHub.

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

#218

Earlier quoted context omitted.

You can do that in Github, but for some reason a lot of reviewers are not familiar or bother doing that. Fixing nits that way or giving a suggestion improves turnaround speed greatly and builds a relationship between reviewer and proposer and the final product/commit(s).

Yes, I have been "suggesting" a lot of one liners, typo, rephrasing, or just simple clean up of code with it. Make it a breeze, it's like playing tidy-up without having to branch out or bother much the author.

I feel stupid. How do I do this?

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

#219

Earlier quoted context omitted.

It’s literally the definition of hazing. But instead of being asked to jump in a pool, naked, while snowing, you are asked to build things a new hire has no business building. Then nit-picked for not knowing things. Literally set up for failure. A better solution is to actually sit with them while they build a feature, show them around the code, and answer questions. You know, treat them like a team member instead of…

There is no hazing because there is nothing personal here. You are not being judged. The code you wrote is, but feedback in a code review doesn't say anything about your competence (though how you respond to that feedback says a lot). If your team is insulting you or saying that you're a bad engineer based on code review comments, that's a bad team. That doesn't mean that ensuring new team members learn the team's st…

I'm sure that is what the frat boys would say when they tell everyone to jump in the pool, in a blizzard... it's not personal.

If you don't suck it up, you're not "one of us" and you need to go. Or maybe they told you to do the wrong thing and see if you point it out. Who knows!?

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

#220

Earlier quoted context omitted.

Doesn't seem hard to me? You could easily run a spell checker on identifiers and comments. It will produce a lot of false-positives, but that can be solved by making the changes optional or using an allow list.

Then please write one. Note that the important part is a good way to mark all those false positives that is simple and doesn't go to far. Just because I want a bad spelling in one place doesn't mean I want it everywhere. As a result I'm going to predict that your tool either results in too much boilerplate needed to suppress all the false positives, or your tool lets pass a lot of things that shouldn't. But that migh…

It’s called cspell https://cspell.org/
Post reply on HN