[HN Gopher] A study of Google's code review tooling (Critique)
___________________________________________________________________
A study of Google's code review tooling (Critique)
Author : vinnyglennon
Score : 225 points
Date : 2023-12-04 15:31 UTC (7 hours ago)
(HTM) web link (engineercodex.substack.com)
(TXT) w3m dump (engineercodex.substack.com)
| dontreact wrote:
| I generally like the tool.
|
| When your code review tool is really nice one unexpected negative
| is that it can create a culture of nitpicking. Sometimes it's not
| worth arguing over small details like variable naming but the
| tool makes it really easy for things to head in that direction.
|
| Sometimes variable naming isn't a small detail. But sometimes it
| is and it's a waste of everyone's time to argue about it.
| shadowgovt wrote:
| It can be tricky to find a balance point here. At Google scale
| (in time-of-maintenance, not space... Code can last for years
| and will be worked on by multiple engineers disconnected from
| the original project), the hard problem of naming things
| becomes real.
|
| I find that it's useful to keep context. Even though one can
| never predict with certainty, _sometimes_ you can be real
| confident that some code is prototype that will be thrown away
| in six months. And there 's a big difference between the code
| that makes up an API layer and the code that implements a
| feature constrained to one module.
| arp242 wrote:
| At not-Google-scale code also often lasts for years; perhaps
| even more so since there's typically a lot fewer people
| maintaining it.
|
| I think the key thing is to ask yourself "is this really
| objectively better yes/no?" before commenting. Not that you
| can never comment if the answer to that is "no", but quite a
| lot of the time when the answer is "no" it doesn't really
| matter and it's just "I would have done it slightly
| different, but both are fine".
| shadowgovt wrote:
| Good point. Making renames a suggestion, not gating (unless
| it contradicts the name in some design doc somewhere that
| another team is relying on not to move) may be the balance
| point there.
| arp242 wrote:
| But "another team is relying on not to move" is an
| objective point, right?
|
| Things like "I find this code very hard to follow, and I
| think it could be made easier" is also objective, and
| even "I don't understand what this variable name means,
| and I think it could be clearer".
|
| I once names a function mkdir(). This created a directory
| tree. In the review it was called "obscure" so it became
| createDir(). Then someone pointed out that "dir" was a
| needless abbreviation so it became createDirectory().
| Then yet someone else pointed out that it's actually
| recursive, and a discussion on the merits of
| createDirectoryRecursively() vs.
| createRecursiveDirectory() was inflicted on everyone. In-
| between there was a side-quest about directory vs.
| folder. Just fucking stick a fork in my eye already.
|
| Was anything of any objective value gained? I'm having a
| hard time seeing it. Well, it makes for a slightly
| amusing anecdote so there's that.
| ryandrake wrote:
| I'd wonder what the rest of that API surface looked like.
| If the rest of the functions were
| longDescriptiveCamelCase() and then you tried to slip in
| mkdir(), then I'd say yes, "stylistic consistency" was
| gained during that unnecessarily difficult back-and-
| forth.
|
| If the other functions were chdir(), remove(), rmdir()
| and so on, and somehow the reviewers picked your code
| change as the time to change it all, then yea, what a
| waste.
| arp242 wrote:
| It was a mix of short and long and there wasn't really
| much "style". The company had taken over the
| maintainership from someone else at some point (before I
| joined) and it was all fairly inconsistent. I don't
| really mind the inconsistencies as such, it's just the
| argueing over nothing that I mind.
|
| This kind of stuff was typical. At some point there was a
| lengthy discussion which prevented rolling out a rather
| important hotfix over: while (true) {
| if (someCond()) break; if (otherCond()) break;
| [..] }
|
| It wasn't even about whether someCond() and otherCond()
| should be in that while(..) condition, it was allegedly
| "dangerous" because it "could loop infinitely". Well,
| ehh, that's the case with any lop innit? That's kind of
| how they work? There was a bunch of instances of code
| like this, "but it's still dangerous". Hmkay...
|
| Oh, and then there was the great "evil incident". I had
| rewritten much of the frontend to this newfangled thing
| called "jQuery" that was all the rage. That was all fine
| and worked pretty well, but suddenly there was a bug
| hubhub about the jslint comment; the keyword to allow
| eval() was "evil" (one of those not-too-funny Crockford
| jokes), so at the top of
| /static/js/app.minified.js?v=12311 in prodiction there
| was something like: /*! jslint: keyword1
| evil keyword2 */ /*! MIT license blah blah */
| minified_js()...
|
| And people were up in arms and there was a big panic
| deploy during lunchtime for this "because customers might
| see this, and it's highly unprofessional, and it's a big
| problem we need to fix ASAP" etc. etc. etc. This was in
| the Netherlands with regular people, not some highly
| conservative part of the world. It was downright surreal
| and bizarre.
|
| What I'm trying to say is that these people were rather
| obsessed with minor details to a point I've never seen
| before or since. Hell, there was a Company Blessed
| IDE(tm) that you _had_ to use. Nothing else allowed. It
| was a complete piece of shit (IMHO) and after a few weeks
| I just used Vim. It worked. I got stuff done. No one was
| bothered by it. Still got comments I shouldn 't be doing
| that...
|
| While I didn't formulate "is this really objectively
| better yes/no?" clearly at that time, I'm sure it's been
| a pretty big influence.
| IanCal wrote:
| > it was allegedly "dangerous" because it "could loop
| infinitely". Well, ehh, that's the case with any lop
| innit? That's kind of how they work?
|
| No? You can clearly have loops that exit. Loops that can
| be infinite should only be so where you really want that
| to happen in those cases / are fine with it. "Infinitely
| until the user does X with no timeout" is a fair one.
| arp242 wrote:
| 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 stupid loop
| and no one even had any alternative. Eventually the loop
| committed.
|
| The thing is, if I had written the conditions in the loop
| body it would have been fine with no comments. People
| were tripped over the "while (true)" and commented on
| that. That's okay, I can just change that, no problem.
| But the kept analysing and thinking about it to the
| subatomic detail. That as really the problem: small
| things tended to escalate to "a big thing" even when it
| was just a small thing. I don't know what it was - a kind
| tunnel vision? I don't know.
|
| I once carefully suggested to paint a wall in a brighter
| colour during a HOA meeting. People objected. Fine, not a
| big deal and just a suggestion, I'm okay with the grey it
| was. They kept discussing it. I back-pedalled harder.
| People started to suggest we need to form a committee. I
| started to run. They asked if I wanted to join the
| committee "as I had brought up the issue". I overtook
| Usain Bolt. I heard they had several meetings. They
| choose the same grey it was before (or something so
| similar I couldn't spot the difference). Not my favourite
| colour or what I would have chosen, but it's fine.
| joshuamorton wrote:
| 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
| should be incumbent on you to prove the loop halts, not
| on the reviewers to prove it doesn't.
| arp242 wrote:
| 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.
| shadowgovt wrote:
| > Things like "I find this code very hard to follow, and
| I think it could be made easier" is also objective
|
| The "I" in that sentence suggests this should be
| considered subjective. And that's I think the cleave-
| point between gating and non-gating: "Other people have
| already agreed on this" vs. "In the moment, I, a single
| code-reviewer, think this name could be improved."
| arp242 wrote:
| It's not an exact science and there's a grey area of
| course, but what I have in mind is mostly code that's
| just needlessly confusing in ways I think very few would
| disagree with. I can't really think of a specific non-
| trivial example at the moment, but just like natural
| language I do think some code can objectively "hard to
| follow", even if that's somewhat vague and not strictly
| defined.
|
| Other factors are if I'm the primary or one of the
| primary maintainers, the standing of the other person
| (also a maintainer or one-time contributor from another
| team), and things like that.
| shadowgovt wrote:
| I can't speak to everyone's experience, but I can say how
| teams I've been on have handled that sort of thing.
|
| When it's not explicitly documented in the style guide
| and it doesn't violate some agreed-upon convention (which
| needs to be gleanable from within the file itself or it's
| not an agreed-upon convention... No "We know it was
| always done like that but now we're doing this," if you
| want to change it, you do the heavy lifting of putting
| together the consistency refactor to bloody well _change_
| it...), it 's a suggestion. Non-blocking.
|
| But taste _does_ matter, so if an engineer is observed by
| their peers to be ignoring too many refactor suggestions
| with no explanation, it becomes an issue with the project
| lead and that engineer might get put on a shorter leash
| (in the form of "These are normally non-blocking...
| Except for Steve, he wouldn't know consistent and terse
| naming if it bit him on the FaceManagerFactoryService).
| It required enough human touch to understand what each
| others' strengths and weaknesses were, so more expensive
| in terms of mind-space, but I think it generated better
| results overall.
|
| (And as the one whose code is reviewed: if you have a
| good reason not to change something and can express it,
| fine! You're a team member too and your opinion on how
| things should be also matters).
| arp242 wrote:
| I'm not talking about style, I'm talking about logic.
| Some logic is easier to follow than others, and as
| mentioned this is not exact and somewhat fuzzy, but
| "every logic is equal to every other logic" is obviously
| nonsense. All other things being equal two "if"
| statements are better than nine "if" statements.
|
| I don't care about style; as far as I'm concerned it's
| "do what thou wilt shall be the whole of the style guide"
| (well, within some obvious limits of reason). I don't
| even care about consistency all that much.
| TheBlight wrote:
| Excessive subjective feedback can be soul-crushing.
| acscott wrote:
| Agreed; after > 20 years of coding successfully, got hit by a
| storm of subjective feedback; it totally ruined any joy in
| development
| TheBlight wrote:
| It didn't used to be this way. A code review used to mostly
| function as a quick sanity check. Now it's basically code
| by committee.
| wubrr wrote:
| 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.
| runlevel1 wrote:
| 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 how reasonable they
| are, and can get clarification of what they actually care
| about versus FYIs/suggestions. As they go, if it isn't
| clear, I just ask them if something is a soft opinion or
| hard opinion.
|
| If everything is a hard opinion and they don't seem
| reasonable, I reach out to someone else (ideally a team
| lead or peer on their team) over a private channel for a
| 2nd opinion. If they also think it's unimportant stuff, I
| ask them to add their own comments to the PR. Give it a
| reasonable amount of time and they'll either have reached
| a consensus or you can roll the side you agree with.
|
| If it's an issue again later and they seem reasonable,
| respectfully push back. If they seem unreasonable, skip
| right to DMing their lead for a 2nd opinion.
|
| 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.
|
| 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. Of course, if you're at a company with a lot of
| disfunction, well... I can understand why what I've
| written above won't work.
| wubrr wrote:
| 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.
|
| And yeah it totally depends on the company/team.
| MarkLowenstein wrote:
| 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 guidelines, either put it in the
| guidelines, or hold your tongue.
| zeroCalories wrote:
| IMO you should never provide feedback that can be
| implemented as an automated check. If you don't like deeply
| nested control flow, then you should catch that with static
| analysis. If you need code coverage, you should require it
| for merging. Implement your check and provide a new PR to
| fix your nitpicks, or shut up. The goal is to put 100% of
| the focus on correctness.
| merb wrote:
| Sadly typos in variable names are not checkable that easy
| zeroCalories wrote:
| 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.
| bluGill wrote:
| 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 might be just that I don't have good ideas: if
| you create a good tool for this I'm willing to be proven
| wrong.
| dgellow wrote:
| It's called cspell https://cspell.org/
| tunesmith wrote:
| This seems precisely backward to me. Programmers are
| humans, not input/output machines, and there's definitely
| a role for encouraging a certain standard of judgment
| that doesn't require tooling to enforce. To argue
| otherwise seems akin to arguing that any bad behavior is
| fine that isn't explicitly banned in paragraph 5
| subsection D. Tooling is expensive, especially for
| smaller teams, and should be saved for phenomena that hit
| the cost/benefit calculations squarely.
| everforward wrote:
| 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.
| layer8 wrote:
| Unlike excessive objective feedback?
| jeffbee wrote:
| Writing readable code is not "nitpicking". It is common that an
| author doesn't see why their parameter name or function name is
| misleading but their reviewer, who comes to the change without
| as many preconceptions, sees it right away.
| dontreact wrote:
| Im not saying it's always nitpicking. I'm saying that
| sometimes it is.
| bheadmaster wrote:
| "Readable" and "misleading" are subjective, and depend on the
| person's "preconceptions".
|
| I disagree that the reviewer comes "without as many
| preconceptions" - they just come with different
| preconceptions, the ones they're used to. Programming
| language is a language like any other, and each person has
| their own style of writing it, and they'd prefer the rest of
| the world to use their own style because it's "more
| readable".
| tikhonj wrote:
| Are you saying there is no such thing as clearer or less
| clear writing in natural languages _or_ programming
| languages? It 's 100% subjective and depends entirely on
| the reader?
| bheadmaster wrote:
| Of course, it's not 100% subjective - rarely anything in
| the world is 100% anything.
|
| But I think that most people consider their personal
| preferences to be better and more readable just because
| they're used to them, so I tend to take the opposite
| attitude as the starting point. There were more than a
| few situations where I've had a coworker tell me "just
| read the code, it's very readable", only to spend the
| next two weeks just trying to figure out how it works.
| Sure, once you figure out how it works and it "clicks",
| it's no longer (that) unreadable, but the fact that I
| have to spend so much time reading the codebase in the
| first place made me convinced that personal familiarity
| is a great part of what "readable" means.
| KptMarchewa wrote:
| I believe overtly verbose, enterprise Java names make
| code _less_ clear in modern times with proper development
| tools.
|
| Entire group of people has to think otherwise due to
| proliferation of that style.
| jeffbee wrote:
| Can't agree here. Often an author is writing code and they
| start with a tentative function, parameter, variable, or
| type name, then as the change progresses the semantics of
| the thing changed but the name stayed the same. Authors
| won't always see this but reviewers are more likely to
| notice it.
| faster wrote:
| I struggle with this. I work with devs who think that a
| function with 3 side effects is fine, and suggestions that it
| would be more maintainable if it did one thing (and even more
| offensive, that the function name should be a clue to what
| the function does) are often met with hostility. In the end
| they're angry and resistant and I'm frustrated. Sometimes I
| just approve it, write a technical debt ticket, and move on.
| runlevel1 wrote:
| Anger and frustration is obviously not an appropriate
| response to constructive feedback offered in earnest, but
| do you have any insight into why they respond that way?
| Like naivete as to why it's usually a bad idea to do that,
| them misinterpreting your intent, or redirected aggression
| from another source of frustration (like workload,
| deadlines, etc.)?
| seanmcdirmid wrote:
| If it truly a matter of readability, then it really isn't a
| nitpick. If it is a matter of "I think doing X is slightly
| better than doing Y, and if you start doing X rather than Y,
| that would improve things somewhat", then it is a nit.
|
| To put it this way, you should use Kotlin vals with get
| methods with getters rather than fun getXXX() functions, but
| this is controversial, so I always suggest it as a nit (Nit:
| maybe use val xxx get() = ... rather than fun getXXX() =
| ...).
| seanmcdirmid wrote:
| you can mark your comment (in text) as a NIT and then unmark
| "Action required."
| dontreact wrote:
| Yeah... but people sometimes dont do this
| ayberk wrote:
| 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++ readability was a much, much better experience. All the
| comments were about actually making the code better, eg, "use
| THIS_MACRO() instead of THAT_MACRO(), because go/...".
|
| I guess I think it's much more about the reviewer, and based on
| my anecdotal experience, the language :)
| tunesmith wrote:
| 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
| opportunity for how to improve this area of the code", that's
| better then acting like you made a mistake in failing to
| change it.
| xKingfisher wrote:
| 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.
| eichin wrote:
| 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.)
| egl2021 wrote:
| Different languages had different pools of readability
| reviewers, so the expectations varied, but readability reviews
| were generally constructive and helpful. I was thrilled to have
| Ian Taylor review my go code.
|
| The non-readability reviewers were usually on your team, so
| there was social pressure in both directions. You wanted to
| learn and conform to the team's norms, and the reviewer
| couldn't be a total jerk about their comments. Everyone was
| generally on their best behavior.
| shadowgovt wrote:
| That ML-sidecar for recommending the actual change is a very
| correct place to put that technology: right in the line where
| someone is already reviewing the code, so they can confirm the ML
| suggestion looks correct before adding it.
|
| I can't overstate how valuable it is for reviewers to give the
| actual change they're recommending alongside the description, but
| it happens too infrequently if we make the reviewer write it
| every time. Actual code is the cleanest language to communicate
| ideas, but it's a time-sink and a mental drain to turn the
| English description into code snippets every time. Automating
| _that_ is precisely the right place to save time for reviewers.
| jdeaton wrote:
| Critique is nice but the ML suggested edits are a waste of
| attention and cant be disabled.
| shadowgovt wrote:
| What's the current miss-rate on them? In principle, it seems
| brilliant, but I can see that value dropping off a cliff if
| more than half of them are worthless.
| michaelt wrote:
| _> What 's the current miss-rate on them?_
|
| Depends how good your code is :)
| hot_gril wrote:
| Pretty low it seems. Occasionally I get obviously bad
| suggestions that are easy to ignore.
| mattrb wrote:
| They can be disabled. Click the settings cog.
| dpbriggs wrote:
| I disagree. They're pretty good at auto-fixing minor issues and
| suggesting best practices.
| jeffbee wrote:
| Satisfaction with Critique among Xooglers is undoubtedly driven
| by dissatisfaction with GitHub PR reviews. GitHub reviews are
| astonishingly bad. The tool is utterly useless for actual
| reviews. After the first round of comments it becomes total
| chaos. Nobody can tell what's been said, done, changed, or
| resolved. It is impossible to believe that the people who write
| and maintain the GitHub PR tools have themselves ever practiced
| code review.
| fishnchips wrote:
| To be fair to GitHub, there has been slow but constant progress
| over the last few years. I would love it to get at some point
| to where Critique was when I left Google (almost 10 years ago).
| charcircuit wrote:
| I don't see any desire for them to actually improve it. It's
| been over a decade and the code you are actually reviewing is
| not even on the page of a PR. You have go to a separate web
| page to see it.
| codeapprove wrote:
| I was on a team at Google that used both Critique and GitHub
| very heavily, so I was able to constantly see the side-by-side
| and understand the pain engineers faced when doing external
| code reviews (as a whole, people actually liked working on
| GitHub).
|
| After I left I created CodeApprove (https://codeapprove.com) to
| bring a lot of Google's best code review practices to GitHub.
| It doesn't give you everything Critique did, but I think it
| brings the same speed, clarity, and focus in a way that's still
| compatible with the rest of your GitHub workflow.
| tuetuopay wrote:
| I get the same feeling when switching between Gitlab and
| Github. The latter is a joke of usability and discoverability.
|
| Tiny details like thread filtering, thread replies not
| appearing as separate comments, "show changed lines" when a new
| commit modifies the subject of a comment, etc. Those add up to
| make gitlab much more usable.
| hyperpape wrote:
| It's funny, because GitLab feels pretty bad. It's slow, there
| are weird places where things require a full page reload, and
| it's extremely aggressive about hiding files that have
| significant changes (yes, small changes are better, but I
| still need to review big ones some of the time).
| ycombinatornews wrote:
| Agree with this. GitHub was good at some point, then Gitlab
| caught up and now Gitlab has been superior in code review
| experience for few years at least.
|
| Comparing between MR/PR versions, meta commands in MR
| templates, and so on.
| yx827ha wrote:
| If you want something similar, check out Gerrit:
| https://www.gerritcodereview.com/ It's open source and used by
| Android and Chrome.
| jasonlotito wrote:
| Yeah, it seems like Gerrit with lots of Google-specific stuff.
| Not surprised. Used Gerrit for 12 years, and loved it.
| nappy-doo wrote:
| Gerrit is pretty crap compared to critique. It has a workflow
| that works for Android, but critique is really much better.
| cmrdporcupine wrote:
| The essential thing is that Gerrit is a swiss army knife
| review tool for _git_ , whereas Critique is able to be
| consistent and fluid because it only has to worry about
| working with the standard workflow in Piper/CitC (and now
| fig I guess)
|
| I agree Critique is much nicer, but mostly because it's
| more consistent and doesn't have to deal with all the
| oddities of git.
| k_dumez wrote:
| What do you think Gerrit is missing compared to Critique?
|
| Graphite[0] is also similar in that space(code review
| platform built on GitHub), the CLI could use some work but
| combined with the web UI it scratches that same itch that
| Critique did for me
|
| [0] https://graphite.dev/
| sparcpile wrote:
| It looks like Critque is a branch of Gerrit. The user interface
| is similar. I assume that Critque is Grerrit with a bunch of
| Google-specific changes.
|
| Gerrit itself is an interesting review tool. It uses Git
| references to manage the review changeset before it is merged
| into the parent branch.
|
| I used it on a project that used Redmine for issue tracking and
| Gerrit for the git repo and review tool. It took a bit to get
| used to how to git to push changes to a Gerrit review so we
| ended up using Git extensions to manage that.
| cmrdporcupine wrote:
| _I assume that Critque is Grerrit with a bunch of Google-
| specific changes._
|
| Not even close. I have another comment where I get into some
| details, but, no, three's no overlap beyond the fact that
| Gerrit pulled some UI and workflow things from Critique (and
| Mondrian before that, the tool that predated Critique)
| dvirsky wrote:
| > It looks like Critque is a branch of Gerrit
|
| IIRC Gerrit is an open source re-implementation from scratch
| of Critique.
| tallowen wrote:
| I'm always surprised to see the totality of support for this
| workflow. My biggest gripes were:
|
| - Owners
|
| - Readability review (or really the 18month queue to get
| Java/python readability)
|
| Although seemingly innocuous, this made maintaining internal
| libraries very challenging. There was no way to update every call
| site of your library in an efficient way. Tools like Rosie were
| added on top so that you could shard your PRs and shepherd many
| PRs through a Byzantine approval process.
|
| Compared to Facebook, I found it much harder to reach the same
| levels of productivity given Owners and readability review. I
| don't think libraries like React could get developed at Google
| given how hard it would be to evolve the API surface.
| jeffbee wrote:
| Google3 is a huge codebase of high quality that moves
| exceptionally quickly, so it just seems like your priors are
| getting in the way. Complaining that a large-scale change among
| tens of thousands of software developers requires a tool
| (Rosie) is sort of the same thing as all those people who
| complain that it's too hard to cope with having millions of
| machines in prod, i.e. the type of people who wash out of the
| company in a few months and then go on to a career of
| complaining about it online.
| marcinzm wrote:
| The same applies to Facebook which is their comparison and
| not some random 10 person startup.
| jeffbee wrote:
| Except Facebook's codebase is an order of magnitude
| smaller.
| tallowen wrote:
| What makes you say that Facebook's codebase is an order
| of magnitude smaller?
| dilyevsky wrote:
| Fb had 100 million loc in 2019[0] while google already
| had 2 billion[1]
|
| [0] -https://www.wired.com/story/facebook-zoncolan-
| static-analysi...
|
| [1] - https://www.wired.com/2015/09/google-2-billion-
| lines-codeand...
| ot wrote:
| That's an apples-and-oranges comparisons, 100M is just
| the size of the frontend/middleware code (Hack), while 2B
| includes _everything_ , including configuration and
| generated code.
| ASinclair wrote:
| > I don't think libraries like React could get developed at
| Google given how hard it would be to evolve the API surface.
|
| That's more of a feature and not a bug to me. Users of a
| library would appreciate it if the API surface doesn't change
| dramatically over time.
| compiler-guy wrote:
| That's the strange thing about the GP comment: Teams can and
| do change their API surface dramatically over time. They use
| the LSC process that the GP doesn't like to do it, and it is
| 99% invisible to the API users. It works, if not exactly
| flawlessly every time, very consistently well, and there are
| dozens of such changes in flight all the time that API users
| barely even realize exist.
|
| Don't know what the alternative would be.
| tallowen wrote:
| I don't think the burden of the changing API surface was
| place on the callers of that API. With core library
| developers updating their callsites, it meant that people
| calling into early versions of React, never had to deal with
| upgrading the react version in their package. As a caller of
| these libraries, it was much nicer to have the React team
| take care of upgrades where possible rather than having to
| deal with it myself.
|
| Furthermore, there were tons of changes that could happen
| across the board at facebook that didn't happen at google due
| to owners files. Lots of people were empowered to make broad
| changes to the entire codebase which in aggregate made the
| codebase better. Coming from Facebook, all the systems around
| owners and readability represented a huge amount of time to
| be able to make changes that should have been easy. It also
| left the codebase with many warts that would have been
| relatively quick to fix locally but a pain to land.
|
| Facebook went in the other direction - there were better and
| better tools for making broad updates
| CalChris wrote:
| Dramatically, no. But then a couple of releases of deprecated
| and then hasta la vista is fine with me if it means progress.
| tantalor wrote:
| Owners and readability are constraints of the version control
| system, not the code review system.
|
| Now, _finding_ somebody with authority to approve is a
| constraint of code review. But that would be true of any code
| review tool. So it 's not really fair to complain about.
| cmrdporcupine wrote:
| OWNERS always made sense to me, but the readability stuff at
| Google I found just... pointless productivity destroyer. I
| can't understand why they've kept it. In fact they made it far
| far worse with the incremental process.
|
| With the crazy interview process, the copious static analysis
| and formatting tools and style guides + style enforcement
| tools, and with OWNERS and team members reviewing things...
| what is even the point? What are they actually checking for?
|
| Back when I started there, I got my C++ readability quite
| easily with just a month wait or so. Then I briefly worked with
| a bit of Go, and encountered my first taste of the incremental
| process, and it.. sucked. When they rolled out incremental
| readability, I never bothered with any other languages.
|
| Luckily, later, I ended up working in the Chromium tree which
| required no such readability status.
| joatmon-snoo wrote:
| As someone who left in 2021:
|
| - OWNERS is absolutely a necessary and important thing, and yes
| it sucked when it made finding an approver hard, but the point
| of OWNERS was to optimize for _local_ ownership. (For non-
| Google folks: think CODEOWNERS files, but
| hierarchical/recursive, so approvers in /OWNERS, a/OWNERS,
| a/b/OWNERS, and a/b/c/OWNERS can approve changes anywhere in
| a/b/c/...)
|
| - I joined in 2017 and it never took 18 months to get
| readability. If it took you 18 months, it was because you spent
| 2 weeks writing code in one of those languages and then didn't
| keep it up. I worked through the stats on this too- by the time
| I left in 2021, there was virtually no delay to enter the
| readability process for Java and it was maybe a few weeks for
| Python.
|
| > There was no way to update every call site of your library in
| an efficient way.
|
| Does FB have something better here?
|
| G had sufficiently many tools for this, IMO: csearch+grep+sed
| for the easy changes, Refaster/clang-tidy for more complex
| stuff (admittedly C++ AST matchers are black magic that few
| people on even the C++ teams understood). Although I do wish I
| had known about Comby before I'd left.
|
| Rosie - the tool for executing large-scale changes, i.e. if you
| made a change to 1K+ files (often 10K+ or 100K+), you needed a
| way to break the change up into multiple PRs - was also an
| absolutely critical part of this. I never found the approval
| process Byzantine - undocumented, perhaps, but remarkably
| streamlined considering that LSCs meant making simultaneous
| changes to code in Ads, Android, Cloud, Geo, Search, Technical
| Infrastructure, YouTube, or however Google breaks down the
| engineering these days.
| zeroonetwothree wrote:
| At Facebook there is no OWNERS so if you make an API change
| you can just update all the code yourself at once.
|
| It makes it much easier to rapidly iterate and improve shared
| libraries.
| colonCapitalDee wrote:
| Yes, and it also makes it much easier for random people to
| break your code. It's a trade-off
| bluGill wrote:
| Either that or it makes it really hard to find the person
| who understands the code you just changed to verify you
| didn't miss anything. I don't work for Facebook, but I'm
| often the person who does large scale refactorings -
| since I know I'm human I want an expert to verify those
| that look at all scary.
| summerlight wrote:
| The owner system is kind of a necessary evil to enforce reviews
| from the domain experts. I've seen lots of junior engineers who
| just want to submit their code and move on and I appreciate
| their productivity, but there have been so many cases where a
| seemingly innocuous change eventually led to a million dollars
| incident and only those domain experts could've detected it.
| And I've still gotten lots of pagers exactly due to this reason
| even with this enforcement thanks to someone able to find a
| relatively lenient reviewer on the codebase with broad
| ownership.
|
| > There was no way to update every call site of your library in
| an efficient way.
|
| There are global approvers. It's not super easy to get their
| approval (you need to go through the large scale change
| process), but getting each owner's approval will be exempted if
| you can get the one.
| joshuamorton wrote:
| > Although seemingly innocuous, this made maintaining internal
| libraries very challenging. There was no way to update every
| call site of your library in an efficient way. Tools like Rosie
| were added on top so that you could shard your PRs and shepherd
| many PRs through a Byzantine approval process.
|
| Ownership is the easiest issue to solve here. Beyond a few
| dozen clients, you're going to run into the inability to run
| tests fast enough, and beyond a few hundred clients, you won't
| be able to sync fast enough, even if you skip all presubmits
| (there will, in expectation, always be a merge conflict in some
| file). 3-stage migrations/rosie are necessary at that point
| _anyway_. Owners is only the problem at like, 5-10 clients, and
| when I made changes at that scale, messaging owners was usually
| fast and effective.
| throwawaaarrgh wrote:
| > 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 written it,
| get out of the way. If you're saying anything that isn't directly
| related to a necessary change to the code, get out of the way.
|
| When I review, if there's no bugs, and nothing that's going to
| affect performance, and it passes tests and works, I approve it.
| If somebody wants mentorship I'll add my thoughts in a comment
| along with my approval.
|
| I don't have this kind of authority, but I would also recommend
| just not requiring a PR for certain changes. Create a guideline
| so that changes which a person couldn't improve by reviewing or
| that have no impact on the working product are just merged. More
| merging, less useless blockage.
| onlyrealcuzzo wrote:
| Code review at most companies is an ego thing, for reviewers to
| feel self-important.
|
| It's a way for senior people to pieces of crap to everyone
| underneath them because that's what they put up with for most
| of their career - and now they feel like it's their turn to be
| a piece of crap back and perpetuate the cycle.
|
| So many industries run on this exact same cycle.
|
| At Google, the company does a good job convincing you that
| you're in the top x% and lucky to be there. People generally
| don't really feel the need to _crap_ on people in code review
| to make themselves feel self-important.
| sanderjd wrote:
| I never know whether I've just been fortunate in my career or
| what, but I've just never experienced anything approaching
| the kind of unkindness that I frequently see people say is
| common. I dunno, maybe it's just me, but I'm skeptical. I've
| always worked with professional colleagues who are trying to
| figure out how to do their best, and have never once seen
| people behaving in this childish way.
| Ensorceled wrote:
| For every unkind reviewer I've encountered in my career,
| I've encountered at least one person who overreacts to
| review comments.
|
| I also agree that both have been pretty rare for me.
| wubrr wrote:
| It really depends on the company/team. Certain companies
| (Amazon), use PR feedback stats as promotion metrics. They
| don't even consider the feedback quality, literally the
| stat 'average number of comments per PR' was one of the
| metrics used at my last team. The only 2 people promoted on
| this team during my tenure there got promoted based mostly
| on their loudness in PRs and team meetings and constant
| pushing for various (mostly counterproductive) 'process
| improvements' - these people didn't actually ship a single
| feature/project on the team. Very toxic place to work to
| say the least.
| daveguy wrote:
| I think PRs are necessary. You can make a very minor change
| that introduces a bug or discrepancy in documentation. But I
| 100% agree with your "get out of the way" philosophy. That
| notice should come with the PR notification for the reviewer!
| sanderjd wrote:
| I appreciate suggestions that are not "directly related to a
| necessary change to the code", and I believe my coworkers
| appreciate mine. It is very useful to say "did you consider
| doing it xyz way / using xyz library?" while also mashing the
| "approve" button. If they say "yep and I decided not to because
| of xyz" or "yep I like that idea but I'm not gonna change it at
| this point" then no harm done. But pretty often in my
| experience they say, "ah, nice, thanks, I didn't think of that
| / I wasn't aware of that, I like that more; updated now", and
| then everybody is even happier.
|
| Another useful thing, in my opinion, is to use your "fresh
| eyes" to make suggestions on what could use to be clarified in
| code or documentation. "This wasn't immediately obvious to me,
| could you add a comment on why it works this way?". The author
| is often too close to the code to see that it isn't obvious,
| because it is to them, because they've already spent hours
| thinking about it. But this should _almost_ always what be
| optional suggestions. (But, rarely, code is _so_ unclear that
| it really shouldn 't go in without being clarified or
| documented.)
|
| But in general I do think it's important to consistently be
| thinking to oneself "my goal is to be useful to my team and
| organization. are my comments achieving that? would it be more
| useful to approve or to not approve this right now?". The
| temptation to be a fastidious editor can be strong - if most of
| us didn't have this impulse we'd probably be doing different
| jobs... - but I find this "usefulness" framing to help me a lot
| in resisting it.
|
| And very strong disagree on not requiring reviews. It is _much_
| better to tirelessly foster a low-friction review culture.
| jsight wrote:
| > [...] while also mashing the "approve" button
|
| +1 - It is easy to forget that it is possible to approve with
| comments. Not all review feedback should block a merge.
| nox100 wrote:
| It is a merge block in some code bases. You need to know
| the code the person said they were going to commit is the
| code they actually commit. Especially when there is
| financial incentive and state actors that want code
| inserted.
|
| In my project we used to be allowed to approve with nits
| but recently they changed it that the code needs a re-
| review for almost any edits. The system has some criteria
| for which it will allow minor edits but it's strict enough
| that I've rarely seen it pass an edit. I assume there must
| have been some incident that triggered this.
| lokar wrote:
| IME this is always driven by lazy compliance people
| pushing the most straight forward way to satisfy some
| audit. Plenty of places subject to strict audit rules
| manage to make small post review edits work.
| dkersten wrote:
| I read it as it's up to the person who opened the PR
| whether to apply the nits, or whether to ignore them and
| use the approval to merge. This is how our codebase
| works: you're free to ignore minor suggestions, but if
| you do edit the code, reviews are dismissed and need to
| be provided again.
|
| Basically, an unaddressed nit doesn't block merging. But
| any code changes will require fresh reviews.
|
| If the change is very minor, it's very quick for someone
| who has already approved to check the last commit diff
| and reapprove.
| yjftsjthsd-h wrote:
| I think it's legitimate to force a re-review for any code
| change, given that programming is one of those charming
| areas where a single character can completely change the
| meaning of something.
|
| That said, if you can read the commits individually then
| final review for something where minor changes were added
| should be utterly trivial.
| jsight wrote:
| Yes, it is pretty common to require re-review if it is
| changed again. But approval with comments shifts control
| back to the submitter. They then have choices:
|
| - Modify it and accept that this will trigger re-review
|
| - Merge as-is... maybe they don't even agree with the nit
|
| - Merge as-is and followup with a new MR under the same
| JIRA. This sometimes makes sense if it gets them into an
| important preprod environment, but they are really still
| working on the issue.
|
| - Merge as-is and follow up with a JIRA on the backlog.
| Maybe they realized that the "nit" is really a bigger
| issue and should be addressed separately.
|
| Regardless, there's value in not blocking the merge for
| non-functional issues.
| lazyasciiart wrote:
| I got a merge blocked last week with the comment 'I think
| this work is more related to "TFS X: prepare for Y" than
| it is "TFS Y".' (I eventually realized they wanted me to
| change the commit message). I hate this fucking team.
| wubrr wrote:
| > 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
| written it, get out of the way. If you're saying anything that
| isn't directly related to a necessary change to the code, get
| out of the way.
|
| But how will I prove my 'mentorship' abilities and seniority at
| FAANG without pretending my subjective nitpicks and lack of
| understanding are critical PR blockers? /s
| __turbobrew__ wrote:
| Hear hear! Nit picking can be soul crushing. Having to argue
| with someone over something which is trivial and does not
| matter is a sure fire way to kill my productivity.
| iwontberude wrote:
| I am as unopinionated as I want others to be. All they need to
| do is commit a patch to my branch and fix it. It won't hurt my
| feelings and I don't need to consent. Let's get past the egos
| and wasted time asking for permission to change things.
| fatherzine wrote:
| 100%, extra points for fitted penname.
| BytesAndGears wrote:
| I like your thought process here, but let me propose an
| alternative. I'm going to assume that we're talking about
| Senior->Senior level reviews here. Junior engineers should
| obviously always get more guidance. But for Seniors, the
| alternative is:
|
| Be extremely picky for PRs from new hires, so that they do
| things the way that your company does them. Make sure they put
| files in the right places, and adhere to the "flow" of the rest
| of the codebase. Also the sorts of style that aren't picked up
| by a linter -- like architecture choices -- should be heavily
| scrutinized to make sure that they fit with your company's
| paradigms.
|
| After a while, they're fully assimilated and you have a
| coherent style so that everyone is on the same page. Future
| code reviews go quickly because everyone makes similar
| architecture decisions and you're never surprised. This also
| makes code easier to read if it all follows the same subset of
| patterns.
|
| I'm not actually sure which way is better, but I think that
| both options have benefits.
| marcosdumay wrote:
| I've received plenty of negative feedback on the internet for
| this (and up to now, discarded all), but I really don't think
| acceptance review is the correct time for guidance.
|
| Yes, juniors need guidance. That means you must read their
| code, and talk to them about it. They also need real-world
| feedback. They do really need you not managing all of the
| interaction they have with the real world. Now, what you can
| not manage is up for a complex risk analysis, but they really
| need you to get out of the way at some point.
| johnnyanmac wrote:
| So, when is the right time?
| marcosdumay wrote:
| At some time you plan for it. Preferably a calm time, not
| one they are waiting a decision from you.
|
| E.g. before they start implementing a feature is better
| than after they finish it.
| withinboredom wrote:
| > Be extremely picky for PRs from new hires, so that they do
| things the way that your company does them.
|
| I think the word you are looking for is "hazing":
|
| > haze (v): force (a new or potential recruit to the military
| or a university fraternity) to perform strenuous,
| humiliating, or dangerous tasks.
|
| I had this at my current job. I almost quit because it was
| downright humiliating to get called an "idiot" in so many
| nice words. I don't do well with hazing... so I brought out
| quotes from text books, papers, and famous authors to point
| out how wrong they were.
| notnullorvoid wrote:
| They are not suggesting hazing. I don't doubt that you
| experienced hazing. Being vigilant with new hires to assure
| assignment on code quality and design is not hazing though.
|
| If someone has legitimate concerns about the design
| decisions made, then they should voice them. However if
| they are refusing to adhere to guidelines, simply because
| they dislike the approach then that's being overly
| problematic.
| withinboredom wrote:
| 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 making them prove their mettle.
| BytesAndGears wrote:
| I definitely didn't mean it like hazing.
|
| I don't see what's so wrong about saying "you used
| inheritance for this relationship, but we have a pattern
| of keeping classes like this separate, since this system
| tends to change frequently. Please organize this like xyz
| module instead."
|
| Just a random example of something that a new person
| might do who is unfamiliar with xyz module and the
| complications there.
|
| I agree that it's good to mentor someone new, but
| honestly I think they still make most decisions
| themselves, and sometimes those decisions don't match
| established patterns that they don't know about. Ideally
| you notice sooner than a PR but I also think that people
| get busy and it's ok to not have time to monitor
| everything a new person does. So sometimes it comes down
| to the code review to notice.
|
| It's not derogatory or hurtful, literally at all, it's
| just pointing on that they did something in a way that
| goes against established patterns, and it's teaching them
| what those patterns are.
| withinboredom wrote:
| 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 I will refuse to make changes
| in my PR if it is cargo culting. You're welcome to open a
| PR to my PR with changes if you feel strongly about it
| though.
|
| Maybe this makes me hard to work with, but so far, I feel
| like it has led to better code and a higher velocity,
| everywhere I've worked.
| Arainach wrote:
| 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.
|
| What you write is not YOUR code. It is your TEAM'S code,
| and the good of the team is far more important than what
| you personally like.
| withinboredom wrote:
| 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.
| Arainach wrote:
| 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.
| withinboredom wrote:
| > 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.
|
| Same. Nor do I consider code review comments personal
| attacks, but anything that _must_ be changed for
| "reasons," _must_ be questioned. Not because it 's
| personal, but because I legitimately want to know.
|
| > Code review feedback such as the following is all
| objective, beneficial to the reader, and not worth
| pushing back against
|
| I'll agree that none of this is probably worth pushing
| back against, but it isn't objective. Very little is
| objective, in our industry.
|
| > This logic should live in [other component]
|
| Why? DDD will say one thing, MVC will say something
| different, though usually they are compatible or can be
| made to be compatible. You can also subscribe to
| hexagonal architectures and that might say something
| different...
|
| There's nothing objective about where code should live,
| except on a hard drive or some other storage medium. I
| would argue that code probably shouldn't live on paper. I
| imagine most of us would agree with that.
|
| > Our RPCs are named as GetFoo, not DetermineFoo
|
| This is a nit. I'd honestly probably ignore it.
|
| > That function name does not make it obvious what this
| code does, please change it
|
| I'd probably ask for a suggestion because it probably
| looks obvious to me after staring at the code for so
| long. That being said, it might be a valid suggestion,
| especially if the code was refactored, but wasn't
| renamed.
|
| > This needs a test
|
| I hope nobody ever says this on my code reviews. However,
| I don't write tests for "obviously correct" code (code
| where the test implements the logic to test the logic):
| such as a function like: function
| returnTrue(): true { return true; }
|
| If I see code that changes "obviously correct" code, then
| a test is warranted.
|
| > This is untestable and needs to be refactored to
| support X
|
| Do you know there is a such thing as legitimately
| untestable code (or at least, it shouldn't be tested in
| traditional unit tests)? Usually at the edges of two
| systems. For example, an API integration can't be tested
| fully, only the known contract from the other system.
| Then you start running into Postel's Law ... things get
| weird. Only if you have some kinds of guarantees with the
| other system (not usually), would I recommend traditional
| tests.
| Arainach wrote:
| 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 style and patterns is hazing.
| withinboredom wrote:
| 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!?
| johnnyanmac wrote:
| No, the word is called "alignment"
|
| >an arrangement of groups or forces in relation to one
| another
|
| If you're being harassed in PRs the peer review system has
| failed. It's a place to make sure everyone is on the same
| page, not judge someone for how they code.
| Eji1700 wrote:
| > If you are holding up the approval because you want someone
| to rewrite the code the way you would have written it, get out
| of the way.
|
| While I'm being pedantic here and get your point, I do think
| it's important that code is following proper styles.
|
| I get that there's a bunch of nitpicky nonsense that goes into
| code review, but making sure it follows the same style so in 6
| months when someone else has to dig through it you don't have
| to teach a totally different pattern/method or figure out what
| arcane bullshit they did is important.
|
| And again, I'm sorta assuming this was implied but wanted to
| throw it out there since it's one of the most annoying things
| i've ever had to deal with.
| ryandrake wrote:
| Style wars are the kind of thing that should just be
| automated away with a source control hook that reformats all
| submissions.
| wubrr wrote:
| This. Any subjective style/pattern agreed to by the team
| should be checked and/or fixed automatically. If you don't
| have lint rules set up to enforce these things - go write
| them instead of commenting about subjective style
| preferences in PRs.
| hn_throwaway_99 wrote:
| Your comment is why I highly recommend using conventional
| comment labels, https://conventionalcomments.org/#labels, when
| doing code reviews.
|
| We've internalized them where I work and I think they're really
| valuable. When it's abundantly clear whether something is a
| nitpick, suggestion, or a blocking issue, because it literally
| has a prefix to that effect, it takes a lot of the potential
| stress and confrontation out of the code review process, on
| both sides.
|
| Note you also don't need to memorize the labels, but starting
| with one forces the reviewer to ask _themselves_ the question
| "Should I really block the merge for this?"
| jrockway wrote:
| To me the biggest time sink in PR reviews is getting the
| reviewer to open the code review. If I ever take over the
| world, I'd make a new OS that locks all activity whenever you
| get a code review and doesn't let you switch applications until
| you click "approve" or "request changes".
|
| I find it really crazy how long people take to do reviews. I
| have Github connected to Slack; if someone needs my review I
| get a message and can have it done within 5 minutes. (I think I
| write the most comments on code reviews of anyone in my
| organization, and I also have the lowest review latency. If I
| can do it, anyone can do it.)
| iwontberude wrote:
| If you link peoples ability to use their computer by how
| quickly they can click "Approve" on a PR, it will definitely
| lead to a break down in review quality and beat you at your
| own personal records of latency and MTTR.
| jrockway wrote:
| I'm sure there would be negative effects, but at least
| things would move forward quickly. This is the worst system
| except for all the others.
| yjftsjthsd-h wrote:
| Move quickly yes, move forward not necessarily. You've
| described a system where everybody hits approve or finds
| something to complain about as fast as they possibly can
| without worrying about little questions like whether it
| should have been approved or whether the changes they
| request are legitimate.
| johnnyanmac wrote:
| Sounds like you work best on a small team if that's your
| focus. Medoum-large ones already have a proven product
| (or proven experience). You don't focus on blazing gast
| iterations at that stage unless you are in early R&D or
| something evergreen.
|
| And well, if you're primary motivation is Financial:
| okay, you're not being paid to move quickly. Or if you
| hope to move up the ladder quickly you gotta deal with
| the office politics. Which includes delays in
| communication. React to those lapses as you feel is
| appropriate to your goals and personality.
| tgsovlerkhgsel wrote:
| Everyone thinks their task is the most important one. If the
| "lock OS until review done" thing were a thing, there's a
| good chance that "lock OS until random bullshit someone else
| wants you to do" would keep you from sending out the code for
| review in the first place.
|
| And even though it's "just a quick thing", if _every time_
| you 're trying to get something done some "quick thing" gets
| in the way, it also gets incredibly frustrating. [1][2]
| Finding a good balance for this is hard.
|
| What helps in my experience is building a reputation for
| small, easy to review changes. I know that I've left reviews
| from one coworker sitting for a long time because I either
| already took a look and knew that it was, or hadn't taken a
| look and dreaded it to be, a huge block of code that would
| require significant comments.
|
| [1] https://www.reddit.com/r/ProgrammerHumor/comments/2rmir6/
| why... [2] https://www.reddit.com/r/ProgrammerHumor/comments/
| pafo1v/und...
| yjftsjthsd-h wrote:
| > I have Github connected to Slack; if someone needs my
| review I get a message and can have it done within 5 minutes.
|
| Do you not do anything else? If I'm working on something
| else, I'm not getting to any new task within 5m (at least,
| not without a net loss of productivity to reload context)
| withinboredom wrote:
| If you're writing code and you get a review request and
| ignore it, you are literally preventing value from being
| shipped while you work on making some abstract idea a
| reality.
|
| Usually, the code review is the very last step before value
| becomes available to the business. There is absolutely no
| reason to delay a code review in this case, none,
| whatsoever.
|
| Even if it isn't the last step, you're delaying the chance
| for a developer to continue working. More often than not,
| when a PR is submitted, there is a period where the
| submitter is twiddling their proverbial thumbs to prevent
| context switching from a reviewer.
|
| So, delaying a review prevents value from being available
| and reduces overall developer productivity for the entire
| team.
|
| Or, keep working on making an imaginary idea into a real
| one while someone else's already built idea sits there
| wasting money.
| johnnyanmac wrote:
| >There is absolutely no reason to delay a code review in
| this case, none, whatsoever.
|
| "I'm in a meeting with business owners". There, now
| either value is delayed or egos from the ones gaining
| value is bruised. Guess which one usually prevails?
|
| We don't have to be this dramatic here.
| yjftsjthsd-h wrote:
| > Usually, the code review is the very last step before
| value becomes available to the business. There is
| absolutely no reason to delay a code review in this case,
| none, whatsoever.
|
| Even if I was just working on another feature, that's
| questionable, because it only works if earlier-stage work
| might not ship at all; otherwise a delay at the beginning
| is the same as delay at the end, and overhead from
| context switching means you should prioritize whatever
| you're already doing.
|
| But where it really gets absurd to claim that code review
| is always the highest possible priority is that I never
| said _what_ I was working on at the time. Maybe I 'm
| working on incident response - "Oh, sorry
| $BIGGEST_CUSTOMER, I know your prod instance is down and
| you're losing thousands of dollars per minute, but I just
| have to pop off for a bit because one of my peers needs
| me to code review a way to make forms load 0.2 seconds
| faster!". Maybe I'm working on a different feature, but
| it's one that will give the company more value (I have
| _been_ in conversations to the tune of "we need this
| feature so we can close with this lucrative customer").
| Maybe the other thing I'm doing is someone _else 's_ code
| review!
|
| Now to be fair, I think there is real value in lowering
| the turnaround time on reviews. I could even see an
| argument for having someone who does code reviews to the
| exclusion of all else (which kinda sounds like the role
| you're playing in your company), provided that's known
| and factored in when handing them work. The only reason I
| think your position is unreasonable is that you've thrown
| out any notion of triage or nuance and pinned reviews to
| the very highest priority above all else.
| withinboredom wrote:
| I hear you, and I agree, somewhat. Hell, my wife calling
| me is probably more important to me than a code review.
|
| That being said, I do make doing code reviews
| _the_most_important_thing_, except that I make it utterly
| transparent when I check for code to review (first thing
| in the morning and after lunch). I also make it clear
| that I'm only spending 30 minutes (max) on the review. If
| I can't review it in less than 30 minutes, I'm leaving a
| comment that the PR is too big and needs to be broken up
| and then not reviewing it (maybe someone else on the team
| is willing to review it).
|
| That's my policy and the team agrees. Other team members
| have their policies as well (some won't review until they
| are also waiting for a review, some simply don't want to
| do reviews at all and only begrudgingly review and some
| are very similar to mine).
|
| I think it is ultimately a conversation the team must
| have to have predictable releases and deployments. If you
| are doing nothing but code reviews for a couple of days
| before releasing ... you're going to have some fun
| integration issues and need quite a bit of manual
| testing.
| notnullorvoid wrote:
| PR review is an async process, you can't expect your
| teammates to drop what they are working on in order to review
| a PR. This is something you can bring up in your stand-up or
| message channel if the PR has been sitting without a review
| then kindly ask "hey X, and Y can you give this a review
| today". Or if it actually is urgent then message someone
| directly and say it's urgent, and offer to hop on a call to
| go through the review together so the feedback can be
| immediate.
|
| Actual priority items are always owned by the whole team, not
| an individual.
| kstrauser wrote:
| Note that PRs are mandatory if a company goes through SOC-2 or
| any other compliance framework. The underlying question they
| answer is "could a malicious or incompetent employee send bad
| code to production without another engineer viewing it first?"
|
| The specific response auditors expect is "no, because another
| human reviews all change requests before they're approved".
| cedws wrote:
| Well said. Ego should stay out of PRs. Your job as a reviewer
| is to _help_ the code get merged, not hinder it.
| ruszki wrote:
| So you don't care about readability, extensibility and
| technical debt then, right? This way of thinking can cause a
| lot of problems long term.
| cgearhart wrote:
| 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 worse than finding it with tests
| is worse than finding it at your desk is worse than finding it
| in the design. Engineering teams should choose an operating
| point in their processes that balances quality and velocity to
| match the business needs of their project. Most often I've seen
| slow PRs caused by stuffing too much of the responsibility for
| software quality into that single step.
|
| In my team we prefix nitpicky comments explicitly with "nit:"
| and it's up to the author to decide what to do with it. We
| minimize trivial nits by enforcing a style checker. We also
| discuss in retro if the PRs are too big to review effectively
| or if PRs are turning up comments on solution architecture,
| because at that point it's too late--the author likely
| should've separately discussed the architecture before it got
| to a PR.
| wubrr wrote:
| Generally agree, but at some companies/teams the ratio of
| 'nitpicky subjective feedback' to 'you have a bug here
| feedback' is like 95:5 while at others it's 5:95. The latter
| function much better.
| poolopolopolo wrote:
| MRs are bad place to align things at scale.
| creshal wrote:
| They should be a last resort for that sort of alignment,
| but it's still important to pull that last resort if it's
| necessary, so you don't end up like Cloudflare and let your
| own standards slip so badly that you end up with a 100%
| preventable multi-day outage.
| zdragnar wrote:
| > In my team we prefix nitpicky comments explicitly with
| "nit:" and it's up to the author to decide what to do with it
|
| This is different from what the OP is talking about.
|
| I've worked at places where staff engineers seem to have been
| rated on number of comments left on PRs... they were
| typically somewhere between nit picky and useless, with the
| occasional person directly contradicting feedback they'd
| given in a previous PR.
|
| Oh, and you had to address every one of them before getting
| approval.
| Reubend wrote:
| > I've worked at places where staff engineers seem to have
| been rated on number of comments left on PRs
|
| > Oh, and you had to address every one of them before
| getting approval.
|
| This just sounds horrible.
| d0mine wrote:
| > rated on the number of comments
|
| It sounds idiotic.
|
| It is unrelated to the question whether short-term (one PR)
| only reviews are preferable over reviews that recognize the
| reality that it is not the last PR ever and therefore more
| long-term view may be useful.
| hot_gril wrote:
| Long-term project health rests on bigger things like API
| design, database choices, or service layout.
| uoaei wrote:
| Hopefully these are in-scope when it comes to accepting or
| rejecting PRs and are not just considered incidental to
| whatever the stated purpose of the PR is. What you listed
| appears to be more fundamental system design concerns so
| hopefully they are candidates for further review.
| hot_gril wrote:
| It's usually out of scope by the time you're writing a
| CL, which is fine because there's design review. The
| issue is about priorities. Nitpicky code review can be
| very expensive, and I'm not convinced it brings
| noticeable benefit.
|
| I've seen team-quarters wasted due to bad large-scale
| decisions. Sometimes the truth doesn't come out until the
| the real thing is in production, and all these extra
| burdens make it harder to change course. I think people
| are starting to get this because the amount of code
| nitpicking or caring about small things in general has
| gone way down.
| throwawaaarrgh wrote:
| > 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 as practical, but not at the expense of
| shipping.
|
| Basically there is a balance to strike. I don't have a
| formula for it, but I do know that merging often ends up
| being more valuable than merging later just to ensure clean
| code. Working features are more valuable than a mess. A mess
| sucks, so we should try to avoid it, but the world runs on
| messes, not clean code.
| danielovichdk wrote:
| Read the Redis code base please.
| notyourwork wrote:
| > If somebody wants mentorship I'll add my thoughts in a
| comment along with my approval.
|
| This is always the approach I take. Especially for junior
| engineers. Stay out of the way, let them merge code with as
| little friction and discouragement as possible. Anything that I
| would have implemented or approached differently I try to
| elaborate on. Even go so far as to write some (most or all) of
| the code in a comment or in a remote branch to share. I will
| still ship the code but let the submitter digest my thoughts.
| Kelkonosemmel wrote:
| I communicate on purpose that code review is never the 99% mark
|
| It's the time to get feedback and improve.
|
| The problem is: if people are stuck and don't learn the code
| review phase will always stay as a long thing.
|
| The whole team is responsible for security, maintainability,
| etc.
|
| I don't care about some code monkey able to write some code or
| fixing something. I care about a team, a product.
|
| Your attitude would not work in my team, which is fine
| lokar wrote:
| IME the biggest blocking feedback to Jr engineers about how to
| better structure things (the most expensive feedback) is around
| making it easy and clean to test the code. Fairly often a
| modest (but straightforward) refactor is needed to get good
| tests.
| dawnerd wrote:
| Yeah no, part of code review is ensuring code quality is
| maintained. If you start letting badly written code fly because
| it passes tests, that'll bite you. If something isn't blocking
| I'll make a note that it should be fixed if theres budget
| knowing sometimes you gotta let things go. But you can't just
| ignore code quality and call it nitpicking.
| withinboredom wrote:
| Badly written, or badly designed? Bad code, to me, would be
| an unrolled for-loop, or a for-loop where a foreach loop
| would do better (though now, we're getting in the realm of
| nit-picking). A PR is waaaaay too late to bring up
| architectural/design improvements (unless it is a WIP PR
| opened expressly for discussing the approach).
|
| I very rarely see bad code in PR's unless it is from a Junior
| programmer, and even then, it's usually because they don't
| know better. Even then, I choose to trust them to do the
| right thing. In most environments, you can even review the
| code AFTER it has been merged. So, if they don't address the
| issue, I can still review the code and let them know that I
| expect to see a follow-up PR addressing it.
| thiht wrote:
| > A PR is waaaaay too late to bring up
|
| No. That's literally the point of a review.
| withinboredom wrote:
| 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 be rewritten.
|
| If you see code badly designed, walk over to, or call the
| developer and say "hey, can we discuss the design of the
| code."
|
| The saying "measure twice, cut once" doesn't just apply
| to wood. Design before you ever write any code.
| DandyDev wrote:
| The alternative is a big design upfront?
| withinboredom wrote:
| Generally, it goes something like this imaginary Slack
| convo:
|
| Me: I think we are getting events from SQS out of order
| and that's why we are seeing some weird synchronization
| issues. What do you think about sending events to the
| regular queue and a FIFO queue at the same time and
| comparing them?
|
| Team: How would that work?
|
| Me: Since we are using a single threaded consumer here, I
| think we can simply override the transport class and
| instead of pulling from one queue per transport instance,
| we set up a feature flag allowing us to pull from two,
| compare the events and if they are different, alerting us
| in the logs.
|
| Team: Sounds good!
|
| You don't need a full design spec, just agreement from
| the team on the approach. There won't be any surprises in
| the review.
| thiht wrote:
| > There won't be any surprises in the review.
|
| There can always be surprises. Sometimes when you see
| something implemented you can get a << click >> as to why
| another solution would be better. It happens to me
| regularly both on the receiving and giving end of this
| and it's rarely a big deal. Most of the time it happened
| to me, I could reuse the functional tests I had written
| and parts of the code anyway.
|
| Reworking on something so that it's better is never a
| waste of time, and it's always better to do it before it
| reaches Prod.
| withinboredom wrote:
| Making a suggestion on how to do something better is
| quite a bit different than saying "this isn't up to
| standard. Do not pass go. Do not collect $200"
|
| We all see these suggestions. That's ok, and wanted. But
| to say "ah, no I think it should be the other way."
| [reject]
|
| That's what I was talking about here.
| johnnyanmac wrote:
| 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 different).
|
| 3. IME there may be multiple reviewers but one primary
| reviewer. Half the time the non-primary ones either focus
| on "is this code going to break your work flow?" or are
| simply there for bookkeeping and isn't the approval
| you're looking for (i.e. A lead on a review that already
| talked about the plans but doesn't have the knowledge to
| review that specific module). Anymore than 1 primary
| reviewer probably needed a smaller PR if possible.
| withinboredom wrote:
| 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 actors to whatever you can imagine. There simply isn't
| a "standard" way to do anything in this industry. If you
| hired me, you hired me for these different perspectives
| on problem solving, and that goes for all of us.
|
| A code review simply isn't the place to enforce your
| unique perspectives on someone else.
|
| Sometimes, there is simply a better way to do things.
| These might even be new ideas in the code base. If it
| looks like it is, hopefully the author will bring it up
| with the team beforehand. If the reviewer changes their
| mind in the code review... well, that's not the authors
| problem. The reviewer should rewrite it and the author
| review it.
|
| Actually, that's exactly what I suggest if someone ever
| tries that with me (usually when I'm the new guy). Please
| submit your own PR showing me how it's done. I'll move on
| to another ticket.
|
| Usually, about half way through their implementation,
| they'll see why I did it the way I did it and approve my
| PR. It only takes one time before they start asking "why"
| instead of assuming they know everything; every line of
| code exists for a reason, after all.
|
| In one case, one specific dude (bless his heart) kept
| blocking PRs after we discussed everything before hand. I
| had to get HR involved, and other engineers. Dude just
| didn't like being wrong and would throw tantrums when he
| was. He eventually got fired after people realized they
| could do what I was doing to stop the nits he said was a
| blocker. Nobody cared if a variable should be renamed
| from "SameFactory" to "EqualFactory".
|
| Ah, but code quality! It's so subjective. High code
| quality, to me, is easy to maintain code. Is the
| intention clear, do the comments reflect reality, is it
| easy to read, but more importantly, easy to change? Will
| changing a line in the module break 15 other sibling
| modules? God I hope not. Dependencies should be obvious.
| And, no, I'm not writing an interface if there is exactly
| one implementation. That's ridiculous.
|
| For some people, they want layers. More layers of
| abstraction than a layered cake on a wedding day. To
| them, that's good code quality! Some people think high
| code quality is beautiful code. The kind you frame and
| put over your fire place to admire with a glass of wine.
|
| There's no objectively "high quality" code in existence.
| JoeAltmaier wrote:
| 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.
| withinboredom wrote:
| 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.
| JoeAltmaier wrote:
| 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.
| ravenstine wrote:
| Yeah, gotta love it when someone uses terms like
| "standard" and "quality" but definitions of those are
| written nowhere. Quality is almost never objective
| either; some cultures like "overripe" bananas and some
| consider the best bananas to have no spots, and those
| picking bananas need a different basis for what makes a
| good quality banana to the buyer. Unless programmers
| diligently document their standards (which they rarely do
| adequately), they shouldn't be surprised if the other
| programmers is dumbfounded by being told their code isn't
| up to standard.
| johnnyanmac wrote:
| >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.
| withinboredom wrote:
| 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.
| Arainach wrote:
| >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.
| withinboredom wrote:
| 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?
| Arainach wrote:
| 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.
| ravenstine wrote:
| Kind of off topic but I'm curious what it is about a
| foreach that you think would do a better job than a
| syntactical for-loop. In my experience, for-loops are
| almost always the better choice, unless the implementation
| of "foreach" is done in a way that is functional and
| recursive. Foreach in the usual sense (like the method in
| JavaScript) provides too little control to the callee and
| is only really of benefit if one really needs to
| conditionally pass in different callback functions.
|
| Besides, that, I mostly agree with your stance on code
| reviews. The way that most programmers do code review is so
| very "waterfall" and they don't even realize it.
| withinboredom wrote:
| > what it is about a foreach that you think would do a
| better job than a syntactical for-loop
|
| It depends on the language. For example, an optimizing
| compiler can optimize the shit out of a for-each loop,
| but can only go so far in a for-loop. For example, in a
| for-loop, you can "skip around" the indices, and even go
| backward after going forward, while in a for-each loop,
| it will always go one-by-one, and the compiler can rely
| on that fact. Even the CPU can probably optimize it
| better since the data access will be more predictable.
| ravenstine wrote:
| Code "quality" is almost always an opinion, and not one that
| is based on anything objective. That doesn't mean that the
| idea of it doesn't have a purpose, but it's almost always
| used as a bat to knock someone over the head with. It's not
| like code can be distinguished as fresh or moldy and rotten
| like fruit. If that were so, then most programmers would
| hardly ever bicker about code quality; it would be self-
| evident, just as any moron can separate the good fruit from
| the bad.
|
| If a unit of code fixes or improves the user experience
| without slowing it down and is memory efficient, then it
| becomes incredibly hard to objectively define said code as
| being bad. Perhaps it ruins the _developer_ experience, but
| then again, many programmers object to things they simply don
| 't like on a philosophical or theological basis. Programmers,
| depending on the language community, may consider anything
| that isn't "object oriented" to be poor quality, but good
| luck finding objective proof that object orientation is
| always better.
| NicoJuicy wrote:
| I care about readability of code, when reviewing a PR.
|
| Not doing it every time, will come back to haunt me later.
|
| Most of the times, the reply on my comment for a more obvious
| variable name is ( not: ) is: yeah, I thought about a better
| name but couldn't find any or something is found in between.
|
| Ofc, sometimes the change is so ugly I'll feel bad because they
| should rewrite it. I'm still struggling with it.
|
| Some people just can't code too. These will come back to haunt
| us.
|
| And will have bugs too...
| sonicanatidae wrote:
| The other side of this coin is without QA, there is no app.
|
| Sure, technically, it can be thrown out the door, and often is,
| in just this form, but it's a shit-tastic mess without QA time
| and the users are getting real tired of half-assed garbage that
| cost twice what the prior package cost.
| ravenstine wrote:
| The fact that your comment is currently at the top of the
| thread tells me that a lot of people go through this, yet this
| is interesting to me because past HN discussions have suggested
| said experience to be an exception. In my own experience,
| blocking (or simply not approving) PRs over nits, out-of-scope
| improvements, and mere personal preferences is the norm. What's
| worse is when PRs get blocked because a person doesn't like the
| pattern the author is using even though it's already one of
| many existing patterns and there's no explicit guideline
| anywhere. I find this happens a lot when lead and staff
| engineers make decisions on what's "considered harmful" but
| don't actually broadcast that decision in an effective way.
|
| What I try to do with my code reviews is prefix my comments
| with "Non-blocking suggestion:" or "Minor question:" so that
| the other person is more likely to know that a particular piece
| of feedback doesn't come with the expectation of needing to be
| addressed.
|
| Otherwise, if the code works, has tests, and isn't objectively
| incorrect, I try to give an approval. If you don't like
| something about it, there should be room in the process to make
| improvements. The idea that "mistakes won't actually get fixed
| later" is a cop out. If your team can't fix mistakes, then you
| should turn in your "engineer" titles.
|
| Also, when programmers do things that others on their team
| disagree with, it almost always comes down to a breakdown in
| communication. Junior developers are a bit of a different
| story, but senior developers not knowing how to write code that
| the rest of the team approves of means that people aren't
| communicating, and it probably means that your system is so
| complex that it's unreasonably difficult to determine the
| proper approach in the first place.
| colonwqbang wrote:
| 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.
| raverbashing wrote:
| Wait... this looks like Gerrit with extra plugins
|
| The experience can be nice sure, but Gerrit is not something I'm
| too enthusiastic about
|
| (and yes, Github is better in some aspects, Gerrit is more
| configurable though and it seems it is smarting tracking "non
| official" merges)
| compiler-guy wrote:
| Gerrit is a released version of Google's code review tool
| called Mondrian--the predecessor to Critique. Critique is
| basically Gerrit 2.0, although Gerrit has evolved somewhat
| independently.
| raverbashing wrote:
| There was also Ritveld (ugh) before Gerrit
|
| And yes, I won't say much about Ritveld besides: ugh
| barrkel wrote:
| Gerrit is a poor man's Critique and it's not nearly as slick to
| use in practice.
| abtinf wrote:
| Are there any examples of successful organizations, maybe even
| with SOC2 compliance, that don't have code reviews?
|
| From my research, I think Netflix might not have them, but I'm
| not sure.
| seadan83 wrote:
| Instead of gating at review time, an org could gate at
| deployment time to achieve that SOC2 compliance.
|
| It appears that Netflix does do CR: "Netflix uses a feature
| branch workflow, where developers create new branches for each
| feature they're working on. These branches are then reviewed
| through a pull request process, where other developers can
| review the code and provide feedback" [1]
|
| [1] https://medium.com/@seyhunak/learning-best-practices-from-
| ne...
| willsmith72 wrote:
| that ui sure is ugly. google really is an engineering company,
| and i love that, but it's still kind of jarring
| qmarchi wrote:
| It's definitely looking better than even a year ago. There's a
| lot of internal UI reworks ongoing to bring things to
| consistency.
| compiler-guy wrote:
| Worth noting that Google engineers had this level of satisfaction
| long before the mostly useless AI suggestions came on the scene.
| Those additions are comparatively recent.
| teaearlgraycold wrote:
| My experience with the AI recommendations was mostly positive
| as an L4 doing Java
| vinkelhake wrote:
| I find them generally very helpful. In a majority of cases,
| it's able to translate my review comments into the exact delta
| that I had in mind.
|
| Source: me, C++ readability reviewer - this means I review code
| from lots of different people, all over the codebase.
| andromeduck wrote:
| It takes way too long though - it's almost always quicker to
| just do it in cider/subl if there's more than 1 of them.
| summerlight wrote:
| I think the AI-powered suggestions are often useful, around
| 30~40% accuracy. The number might seem underwhelming. But when
| I work on code written in unfamiliar language/frameworks, this
| saves lots of my time because it gives some hints on how a
| possible change looks like and which keyword I need to look up
| for.
| SpaceManNabs wrote:
| What is wrong with bazel?
| tgsovlerkhgsel wrote:
| Bazel is a build system, not a code review system. Entirely
| different thing.
| SpaceManNabs wrote:
| You should open the article. The very first content in the
| article is someone praising critique and also saying they
| wont use bazel.
| aerzen wrote:
| It's not easy to setup and has a steep learning curve. But
| it's great to use afterwards.
| codeapprove wrote:
| Shameless but relevant plug: I built CodeApprove
| (https://codeapprove.com) to bring the best parts of the Critique
| workflow to GitHub PRs. Obviously there are many things that
| don't translate (Google doesn't use git, and it has insane CI/CD
| integration) but CodeApprove gives you the same workflow. You
| always know which conversations are resolved, which PRs need your
| attention, etc.
|
| Feel free to reach out via email if you're curious.
| mtlynch wrote:
| I'm a former Google developer and current CodeApprove customer,
| and I just want to endorse CodeApprove. They're the closest
| thing I've found to Critique outside of Google.
|
| The UI is really straightforward, and Sam (the founder) has
| been responsive to feedback.[0]
|
| I have no relationship with the company except just being a
| satisfied customer.
|
| [0] https://github.com/codeapprove/feedback/issues
| pokstad wrote:
| "Write Small Changes" - a software org that embraces this will go
| far.
| hyggetrold wrote:
| I think it's funny that devs will go through all kinds of untold
| suffering when it comes to code reviews...stacked pull requests,
| waiting days (or more!) for a review, all kinds of nitpicky BS
| that is a lot of time and effort to rework by the time you have
| the code all written.
|
| But suggest to people that they pair program and that it's a
| real-time review that obviates the need for formal async review
| and folks want to murder you.
| LeafItAlone wrote:
| I wonder if there are studies on the effectiveness of pair
| programming vs code review and bugs caught.
|
| Anecdotally, I catch more bugs doing code review than pairing.
| When pair programming, we naturally start to think similar
| thoughts, so it doesn't get a great real second set of fresh
| eyes. Even when I have pair, I like to do an alone-time code
| review and things almost always pop up.
| hyggetrold wrote:
| _> I wonder if there are studies on the effectiveness of pair
| programming vs code review and bugs caught._
|
| Whenever folks ask me this, I want to turn it around: are
| there studies on the effectiveness of working solo vs pair
| programming?
|
| _> Even when I have pair, I like to do an alone-time code
| review and things almost always pop up._
|
| Agree - even with code that's been mostly paired on it can be
| helpful to let it sit and get additional review.
|
| In my experience, the killer combo for pairing is to combine
| it with test-first programming. The tests help create a
| shared construct for discussion and understanding.
|
| I freely admit I have a very strong pro-pairing and pro-test-
| first bias. But in my defense it's based on almost two
| decades of experience.
| corethree wrote:
| Google could easily release a GitHub competitor. But they don't.
| Similar to how they invented and failed to popularize the LLM.
|
| It's like the company is run by both geniuses and clowns.
| singron wrote:
| They had Google Code. I think it might have first been
| deprecated before Critique was popular (the prior code review
| tools at Google weren't quite as beloved). Also since then,
| buganizer was made partially public facing. It sounds silly,
| but they could relaunch a software forge suite and it would
| almost definitely be better than GitHub, and they could cross
| sell cloud and probably not even have a free tier.
| slalomskiing wrote:
| That sounds impressive but does there exist a bad code review
| tool?
|
| Just thinking I've used probably 4-5 different ones and never had
| any issues
| mkrishnan wrote:
| critique is a worst tool I have ever used.
|
| example 1: - a engineer caused multi-million dollar loss in ad
| revenue due to change which passed review with flying colors. and
| he is the only engineer whose name ever put in a post mortem
| document. - same engineer few months before this incident forced
| a junior engineer to waste 2 months by making him to stupid nit
| picks. after a month of addressing nit picks, the junior engineer
| showed a slight resistance. then this engineer removed himself
| from the review list (but this engineer is the only one who is
| not manager in that code owner list) and made the junior engineer
| to beg on his knee to provide approval
|
| example 2: - a engineer in my team in double click ad always puts
| tons of nit picks. he got easily promoted.the justification gives
| but the leadership that the engineer created extremes value. the
| leadership was from double-click acquired by google few years ago
| and they are extremely in-competent.
|
| example 3: - there is a tool in google which analyses code review
| comments and those reviewers are very argumentative are given
| badge that they are very throw in review and I have seen those
| stupid badges counted during promotions.
| noname120 wrote:
| I don't see how examples 1 and 2 are related to Critique. Then
| example 3 exhibits a tenuous link to the topic but it's unclear
| how exactly this problem is specific to Critique.
| dpc_01234 wrote:
| I don't really see anything miraculous here, other than "GitHub
| PR review UX sucks in comparison". Things like AI help and extra
| linters etc. are something that a each project has to figure out
| on their own, and e.g. I maintain projects that use Nix dev
| shells to optimize and streamline the dev experience by a lot,
| even before things hits the change review stage.
|
| Github's PR review flow just breaks down for anything non-
| trivial. People have been complaining about it for years and
| suggesting a lot of relatively low hanging fruit improvements.
|
| The rest is about the culture of CRs some of which could benefit
| from built-in support, but some doesn't need it.
| danjl wrote:
| The biggest part of a smooth code review process are the human
| behaviors and best practices. There are a half dozen code review
| tools that work just fine. Both the submitter and reviewers have
| important roles which make the process work smoothly: small
| reviews, 80% of reviews should just be "LGTM", avoid nitpicking,
| code reviews have higher priority than your own development,
| etc.. Really, if you have good code development guidelines, and
| good developers, that solves most of the review issues. The high
| order bit is knowing that every piece of code will be reviewed.
| Any good programmer who knows their code will be reviewed avoids
| doing bad stuff.
| cmrdporcupine wrote:
| Yes, Critique (and Gerritt) is a good tool. Yes, Googlers tend to
| behave well in code reviews and conduct them decently, but..
|
| The reality is that this kind of survey is a) self-selecting for
| the people who adapted well to Google's process, and b) not
| really measuring for productivity, just satisfaction with tools
| and some aspects of the (assumed given) process.
|
| What I personally found is that Google is the kind of place where
| you can only truly be productive if you have the kind of brain
| that can fork off N work tasks at once and then fork/join on all
| of them.
|
| _If_ you 're the kind of person that does better when going down
| the whole mineshaft on one problem at a time, you'll be
| _screwed._ Because you 'll be constantly spending the bulk of
| your time sitting waiting for review approvals, or getting your
| account added into some ACL somewhere, or getting sign-off on a
| PRD or design doc, launch / security approval, or some other form
| of coworker approval. The only way to make progress is to do a
| bunch of those kinds of things at once and juggle between them.
|
| After 10 years of battling it, I realized it's not for me, no
| matter the $$.
|
| (There are some other things that Google does exceptionally
| smartly, though. The monorepo and the way they handle third party
| deps [check them in and only allow 1 version for whole company])
| I think is one. Both of these things seem crazy at first, but
| they are amazing at minimizing whole classes of complexity around
| versioning and dependencies that add mostly needless overhead to
| the job of engineers)
| bvrmn wrote:
| Topic article basically shows screenshots with UI quite similar
| to Gerrit. It would be really great to get even a small glimpse
| of details from ex-googlers instead of "Critique is so much
| better than Gerrit".
|
| We have a kind of big repo for a single product with multiple
| teams working on it. For my company I've configured a few Gerrit
| QoL things:
|
| * Submit rules with different sets of required reviewers per
| directory.
|
| * Keep review labels on trivial rebases.
|
| * CI could be triggered manually by comment in a review.
|
| * Submit requires CI verified label.
|
| Gerrit has a nice query language to search for reviews.
|
| On top of that Gerrit provides patch-oriented workflow backing
| slick review process and keeps history clean.
| cmrdporcupine wrote:
| I used both Gerrit and Critique. There are some similarities,
| and some configurations of Gerritt can be similar. But...
|
| Critique is not git based. And so a lot of the complexity
| around rebasing & commit management is just not there. Nor is
| the stuff about patch-oriented workflow, etc. It's a much
| simpler mental model (central monorepo built on a fork of
| Perforce, though they've added some DVCS stuff with 'fig' onto
| that since), and consistent across the whole giant monorepo.
| Some of the pleasantness comes from that. Some of the decisions
| and twiddly knobs in Gerrit probably come from Google teams
| wanting some of the Critique (or Mondrian before it) workflow,
| but on top of Git.
|
| The UI in Critique is somehow a little more consistent and
| fluid and attractive. Many of the keybindings in Gerrit come
| straight from Critique, and _having_ keybindings is one of the
| things that makes both of them much nicer to work with than the
| crap in GitHub, etc.
|
| Gerrit is also extremely configurable, as you've pointed out.
|
| The query language is one thing they have in common with each
| other, and it's very nice.
|
| In my last year at Google I went back to working in
| Google3&Critique after working in Gerrit & git for some years,
| and I quite enjoyed it.
| rapfaria wrote:
| > Unresolved comments represent action items for the change
| author to definitely address. These can be marked as "resolved"
| by the code author themselves when they reply to the comment.
|
| I've always told the team that whoever comments is the one that
| marks it as resolved (we use github). Otherwise they just gotta
| go and open every single comment again, and sometimes the code
| author doesn't even it address on big changes.
|
| This assumes the reviewer will comeback later to re-review it, of
| course.
| yegle wrote:
| 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 clean up your review queue.
| dgellow wrote:
| I never thought about this but it's a fantastic idea. Would
| love to have something like this for GitHub.
| makerofthings wrote:
| One thing I like about it is that reviewers can suggest changes
| and you can accept them inline. Makes it really easy to deal with
| nits.
| eftychis wrote:
| 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).
| bbu wrote:
| It's not integrated well into the UI and thus a big hassle to
| use. Much easier to just checkout the branch and create a
| commit (or just write a comment...)
| yohannparis wrote:
| 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.
| froh wrote:
| I feel stupid. How do I do this?
| k_dumez wrote:
| joined a startup recently whose founding team was entirely from
| FB (who used Phabricator internally), and Google (who used
| Critique internally).
|
| They missed those code review tools so much they just rebuilt it
| themselves lol.
|
| I was initially skeptical but after using it for like 3 or 4 days
| it's going to be hard to go back to vanilla git tbh
|
| https://graphite.dev/
| mstachowiak wrote:
| Shameless plug - I'm one of the creators of GitContext
| (https://gitcontext.com), a code review tool which has drawn much
| inspiration from Critique and others, and would love feedback
| from anyone who's interested in kicking the tires. We just
| launched in private alpha.
|
| We're putting a maniacal focus into the user experience of code
| reviews, which we feel is overlooked by most tools. Many of the
| features of Critique that developers enjoy have been included in
| our first release... - A focus on only the latest changes - A
| familiar, side-by-side diffing interface - show 'diff from the
| last review' by default - Tight integration with other tooling -
| 'Action set' tracking - we allow you to pinpoint and assign line-
| level issues to relevant team members and track who's turn it is
| to act - Satisfying gamification - plenty of buttons that go
| green and even some fun visual rewards for merging
|
| Additionally, we've layered in... - A beautiful, modern UX that
| provides light or dark mode - Comments that never become outdated
| and reposition/evolve with the review - Smart version tracking
| that handles rebases, merges, and force-pushes gracefully -
| Progress tracking that allows you to see what each participant
| has left to complete down to the file revision level. - A real
| focus on trying to get turn tracking right
|
| We're just getting started and have a ton of ideas we can't wait
| to layer on. If anyone is up for giving it a try, we're actively
| seeking feedback. If you mention 'Hacker News' in the waitlist
| form we'll let you in right away.
___________________________________________________________________
(page generated 2023-12-04 23:01 UTC)