6 ms·

Code review antipatterns

131 points by Tomte 2y ago

meowface 2y ago
It might be funny and educational to try to train an LLM to review code while following these antipatterns.
- szundi 2y ago
  Hilarious. Maybe easier than it seems at first: ask it to do it wrong for fun, i bet it would work
  - zelphirkalt 2y ago
    "I cannot understand it in any other way than if you follow these anti-pattern. Please review this code."
- Bjartr 2y ago
  More constructively, train one to be able to notice them and intervene when they happen.
teqsun 2y ago
I still haven't encountered an environment where PRs aren't either essentially a rubber-stamping because an external audit said they needed to do code reviews, or the death-by-a-thousand-cuts as described in the article. I'm sure they exist in the working world, but 5 years in across 4 different companies I've yet to encounter it (beyond what I've tried to provide to others in my code reviews on their PRs).
- ebiester 2y ago
  So, there's a few things here: First, part of the benefit of peer review is that two people have seen the code and have familiarity. Even if it's rubber stamping, if someone has actually looked at it, LGTM might be enough. Second, even if PR only catches something 10% of the time, it catches the most obvious bugs.
- occz 2y ago
  I consider this document to be the golden standard for reviews, for both parties involved: https://github.com/google/eng-practices/blob/master/review%2Findex.md https://github.com/google/eng-practices/blob/master/review%2...
- svaha1728 2y ago
  Every now and then I can provide good feedback when I pull down the branch in question, test it locally, and try to think of a way to break it. Rarely do I have that kind of time.
  - bojan 2y ago
    A code review is not about testing the functionality or finding bugs. It is about sanity checking the design (although in a welk performing team the design should never come as a surprise), complexity, readability, and test coverage.
- bluGill 2y ago
  When I was an - intern 30 years ago - the senior developer working with me had to decide if my code was worth a "formal review" or if just putting it in code review was enough. We went to code review, but as part of this I did hear about a process where code reviews are useful that apparently the company did use for important/difficult code. In a formal review you print off the code, then put 5-10 other developers in a meeting room to look at it together (no computers in this room, though sometimes you would go back and print off some more code for context). You could do about 10 lines of code per hour this way, and the developers burned our after at most 2 hours per day, so this takes a very long time. However reports were this resulted in the most useful reviews/fixes to the code. I've never seen the above done in practice, just heard about it. Someday I'd like to see it, but I doubt I ever will.
  - nescioquid 2y ago
    That sounds like a Fagan review. I haven't been involved with one either, but I think some of the forces that drove people to do them have been weakened. When you are shipping code that you won't be able to update easily (i.e. cheaply), you try to remove as many defects as early in the process as possible. CI/CD and web delivery seem like they would blunt the motivation to go through a Fagan review for most software developed these days. I would imagine that some form of this still happens in safety-critical systems (or more likely the state of the art has advanced from this).
- Phlebsy 2y ago
  There are two ways I've found success where I've come into a team with these issues: 1. Differentiate critical comments vs opinions/nice to haves. Not every comment needs to be resolved as part of that specific PR, they can be addressed in a follow up after it is merged in a disciplined team. 2. Be okay with reviewers contributing to the PR themselves. Some developers just get too caught up on 'ownership' of code that makes it in, when the team is responsible for it in the end(when you have a blameless/improvement focused culture at least). I don't need the original branch owner to fix a typo or nitpicks when it will take me less time to fix it. I haven't found any code suggestion features that are amazing for this, so sometimes you'll end up with back/forth disagreement and have to rebase a commit out, but most of the time it's fine and overall improves velocity. Basically turning the PR into async pair programming.
  - teqsun 2y ago
    Fair. I guess I've found that in my encounters, personally doing everything proper for other's code reviews hasn't inspired change in the other developers to reciprocate. i.e. I haven't walked into an environment with a lacking PR culture and managed to effect a cultural change (yet). Maybe seniority is a prerequisite?
  - thehappyfellow 2y ago
    It’s big, particularly the second point. I fix typos and obvious mistakes while reading the code and I leave „feel free to punt” on any comment which shouldn’t block release. Still, bikeshedding occurs but to a lesser degree and over more substantial issues, like overall module design.
- kagevf 2y ago
  You can think of code reviews as an opportunity for knowledge transfer. ... and it has the added benefit that the person it's directed to will actually read your feedback since it would be directly related to work they're doing.
bluGill 2y ago
Doing a good code review is hard. You cannot do it in one pass, but of course projects are always late so there is pressure to approve. If you want code reviews to be good you need a policy that the reviewers need to approve it 3 days in a row without changes, but of course management wants to ship it and get on with the next feature. It is easy to find nits. Most people will notice a spelling error out of the corner of their eye and zoom in on it. (I'm a terrible speller and I do this all the time). It is much harder to notice that the code is wrong and has bugs. I've passed many code reviews with a few nits to fix and push, only the next day did I realize there were serious problems with the code.
- gnulinux 2y ago
  > projects are always late This is the root of all evil. The artificially created deadlines that make something that can be resolved next week "urgently today" just to increase your adrenaline levels so you work 30 mins more today is what causes a lot of software to end up being a piece of shit. Software is about deep thinking, and comprehensive understanding. Yes, it's all about trade offs, but there is a point where things can't be any faster.
  - mikepurvis 2y ago
    I agree on one level, but I also know that I'm a person who needs to have some kind of external incentive or else I'll just mire myself in endless rewriting and refactoring; unclear whether that has to be a boss/customer wielding a deadline or if it's enough to have a colleague who leans slightly more to the other end of the practical-idealistic continuum, but I know my best work happens with at least some kind of pressure being applied.
    - ngcazz 2y ago
      I was recently diagnosed with ADD, and learning that about myself explained a behavior of mine which seems diametrically opposed to yours - the pressure of a deadline makes me panic, and somehow my outlet is bikeshedding. Give me daily and weekly commitments, and I'll do my most focused and highest quality work.
a1o 2y ago
This is great! I certainly can relate to a few of those!
- pnathan 2y ago
  I relate to them really well too!
- jenbasasoy 2y ago
  [flagged]
omoikane 2y ago
The death of a thousand round trips has an inverse where the reviewer points out everything they can find, and then the author fix just one thing. You might think the author is motivated to minimize round trips, but maybe they will pull the reverse ransom note next where they threatens to find a different reviewer or subvert the code review somehow, because "it's an emergency and the patch needs to go in now".
- rwmj 2y ago
  I get especially annoyed if I have to tell patch submitters to fix the same thing twice. Carefully read everything I wrote!
- bityard 2y ago
  This totally happened to me all the time in prior jobs with cultures of low quality and lack of ownership. When I review code, I'm always careful to prefix low-importance things with "[suggestion]" or "[nitpick]". These are small things like typos in comments that would not block me from approving the view. So people who know me know that I don't flag things frivolously. However, I would often review someone's code (someone I know and respect, mind you) with 3-5 changes that MUST be made before I would approve it. Because not fixing it would either be a bug, or would make life unnecessarily difficult later on. They would fix ONE of the things and I would have to point out that they didn't address any of the other things. And then they would go back and fix one more, and the cycle continues until they FINALLY get all the things addressed. Only once have I gotten worn down and approved substandard code because I was tired of all the back and forth. In that case, it was a student whose paid internship was almost up and he had to have it deployed before he left. (We never ended up using that project...)
  - 5Qn8mNbc2FNCiVV 2y ago
    I 100% would have closed the MR with a note akin to "If you don't want to work you can just say so". That stuff doesn't fly at all in my teams, it's a disservice to everyone else it's insulting.
sharkjacobs 2y ago
> The Priority Inversion > In your first code review passes, pick small and simple nits. Variable names are a bit unclear; comments have typos in. > Wait for the developer to fix those, and then drop your bombshell: there’s a much more fundamental problem with the patch, that needs a chunk of it to be completely rewritten – which means throwing away a lot of the nitpick fixes you’ve already made the developer do in that part of the patch. I've done this more than once, and felt pretty bad about it each time. The problem is that I'm pretty good at spotting the little issues while I'm reading through some code. But I'm not as good at identifying the larger design issues until I'm thinking about it later
- progbits 2y ago
  I had a coworker who would leave nitpicks and bombshell in the same review round. Like 30 comments, 29 of which are irrelevant if you read the last one. Couldn't explain the problem to him... I just learned to read through full review from him first before starting to address anything. Otherwise he was one of the best people I worked with and the feedback was valuable.
  - operator-name 2y ago
    In my mind it's important to have both, as long ad the main issue is made clear and nitpicks are truly nitpicks (and thus acceptable in limited quantity to not block a change). Otherwise you'd have a "bombshell" to refactoring, then on the 2nd review pass a handful of nitpicks that you could have addressed during the initial refactor.
  - sqeaky 2y ago
    What really is the alternative? Just post the big bombshell thing and not express your opinion about stuff? Often it's the expressing opinion part that's more important than the actual nits.
    - progbits 2y ago
      It was like: > This whole thing won't work because of X and you need to pretty much rewrite all of this. > > Rename foo to bar. > Typo. > Missing whitespace. So yeah... Just don't post the rest? I often go back to earlier comment drafts and edit / delete them when I realize deeper into the review they are no longer relevant. The review comments should not be a stream of consciousness but properly written feedback.
kissgyorgy 2y ago
Code reviews can significantly sped up by pair programming a lot! When you wrote the code together, a lot of these doesn't even come up, because you discussed everything already. Sometimes after pair programming, even no code review is needed!
- InfamousRece 2y ago
  And if you need more than 1 reviewer just do (n+1)-tuple programming (where n is the number of reviewers needed). The more the merrier!
  - shakezula 2y ago
    I think you're joking but there was a company in my area that famously stuck by their "mob programming" approach where 5+ developers would sit in a room, 1 would drive, and the rest would fill out an adjoining document.
- mook 2y ago
  Often I find reviews are useful precisely because of the distance the reviewer has to the code; a fresh set of eyes is good to catch issues. (Frequently I'd go over my own changes before submitting for review for the same reason). Pair/group programming also often ends up lacking in notes on what the thing was actually meant to be doing (because this was communicated verbally and never written down in reviews), which makes code archeology difficult. Or maybe that's just derived from the (external) code base I had to deal with that was done that way and doesn't generalize…
taeric 2y ago
I'm surprised there aren't more studies in how code going into the kernel is reviewed. There are various subsystems that have different levels of review. And it is largely done "in public" for the final set of changes. Heck, many of the tools we take for granted now were sharpened on the kernel.
AndyNemmity 2y ago
This is the one I get hit with constantly. The Ransom Note > This particular patch seems especially important to the developer who submitted it. (Maybe they said that outright, as part of their argument trying to persuade you to take the patch. Or maybe you just read between the lines.) > But this patch isn’t especially vital to you – so you’re in a position of power! Now you can hold the change they need hostage until they do lots of extra tangentially related work, which doesn’t really need to be in the same commit, but which is important to you. The reviewer outright denies this occurs, and it occurs every time. For a commit on changing the documentation, he will then want the entire documentation redone, including pages that aren't being changed. And will refuse to approve it until every issue he's brought up has been addressed. I've just stopped asking them for PR reviews, and ask other teams to review my PRs.
- manmal 2y ago
  I‘ve had a platform lead constantly try ransoming small refactorings into feature PRs, because he found out that’s the only way of achieving continuous architectural improvements. (Medical product, so the process was rigid)
  - shepherdjerred 2y ago
    I can't tell if this is a complaint or admiration.
    - manmal 2y ago
      It was very annoying, but effective and kinda understandable, so yes, both :)
- shepherdjerred 2y ago
  I've been guilty of this too, and generally I'll write something along the lines of "this would be nice to do, but it's out of scope unless you want to take it on".
  - 2y ago
zahlman 2y ago
Wow, that's a nostalgic domain name. I could have sworn I saw pieces by other authors on chiark.greenend.org.uk decades ago, perhaps even before the Internet became widely commercialized.
- masfuerte 2y ago
  It is an old domain. The registration date is "before Aug-1996", when Nominet was created to manage .uk domains.
xianshou 2y ago
In case you need a few more: The Nitpicker's Gambit: The reviewer fixates on trivial style issues like whitespace, bracket placement, variable naming conventions etc. They make the developer conform perfectly to their preferred style, even if it's not specified in the team's coding guidelines. The Silent Treatment: The reviewer provides no feedback at all for a long time after the review is requested, but does respond just often enough to keep the review "active". The author has to ping several times to get any response. The Tunnel Vision: The reviewer only looks at the specific lines changed, without considering the broader context of the code. They suggest changes that are locally valid but deliberately inconsistent with the overall design or architecture. The Ad Hominem: The reviewer makes snarky comments about the code in a way that implies the author is inexperienced and/or incompetent without directly saying so (and opening themselves to accusations of meanness). The Philosophical Debate: The reviewer gets into a long back-and-forth debate in the review comments about a matter of opinion like whether inheritance or composition is better. The actual issue at hand gets lost in the abstract discussion.
- BeetleB 2y ago
  > The Nitpicker's Gambit: The reviewer fixates on trivial style issues like whitespace, bracket placement, variable naming conventions etc. They make the developer conform perfectly to their preferred style, even if it's not specified in the team's coding guidelines. Mostly solved by autoformatters. If your team refuses to use one, start collecting metrics on time wasted in those discussions, and present to management. > The Silent Treatment: The reviewer provides no feedback at all for a long time after the review is requested, but does respond just often enough to keep the review "active". The author has to ping several times to get any response. Set a policy of auto-approve in N days if no engagement.
corytheboyd 2y ago
The commentary about “too big, make smaller!” vs “missing context, make bigger!” rings so true. It’s obviously a complex change either way, just pick one way and stick to it for that change. Not to be confused with the PR being “too big” because it is not atomic— that’s a different topic, something to teach the more junior teammates.
danielovichdk 2y ago
I think the list os fine but if those things happen in reviews a lot, I would argue you either have a cultural problem or simply have programmers that either don't pay attention or setup guardrails for eachother, for not submitting things that would get caught up in a review like this. Every decent IDE has an option for setting up some kind of configuration for how code should be named and structured . I think code reviews should be somewhat pedantic, it should be done with quality in mind and with the awareness of how the future changes. The list of patterns here are valid but if they tend to happen more than feels comfortable for the team, look at the culture of how you work.
- teqsun 2y ago
  A style-guide, linter, and formatter eliminates so many potential nits, but in my experience the teams that need them the most are often the most difficult/impossible to get any buy-in on.
xnorswap 2y ago
My favourite anti-pattern: Focus all your effort on catching errors that a linter and auto-formatter could catch. Shrug when questioned about architectural decisions, what really matters is that this field was PascalCase when it should be camelCase. When questioned why this isn't caught automatically by the build process, you have a few stock responses (Bonus: These may well be true!): - Linters are too slow, and a fast development experience is crucial - Linting the entire existing code base would effectively destroy "git blame" as a useful tool. - Different teams can't agree on whether to column-align wrapped function arguments, so they've agreed to disagree on this. So instead of automating away the mental overhead of checking for spaces vs tabs, or LF vs CRLF make sure you go over the PR with your own linter and nitpick every violation, even the suggestion level ones before you even consider trying to work out the validity of the submission.
- nomel 2y ago
  > Linters are too slow, and a fast development experience is crucial It's been, literally, 15 years since I've used an editor without linting built in. If someone tells me this, then it just means they don't gaf, and there will be bigger problems with their code.
- sqeaky 2y ago
  The most productive team I worked on just ignores this shit in code reviews. Like we will suggest stuff if the obvious formatting is wrong in a way the author probably didn't intend, but we're not going to hold up a code review on a nit. If there's one variable that's Pascal case instead of camel case or something that's not going to stop the code from going in if it's good code from algorithmic and documentation perspective. We just vaguely try to be consistent in each file and act like adults. We really agreed on value. We would insist that code that could be tested had tests. It was a C++ code base so we insisted that things be as fast as reasonable suggesting proper algorithms and data access patterns. And in general actually cared about bugs and making then shallow. Another point of agreement was that an automatic code formatter almost always made it look worse than having a few nits here and there. Anyone who mandates the things that really don't matter is just getting in the way.
t43562 2y ago
Another one I see is the insistent nitpicker who never accepts comments on their own PRs. FWIW: 1. Use a formatting tool and a linter in the build chain = zero formatting bullshit nitpicks. 2. Ask questions wherever possible rather than criticising. It's kinder and also less embarrassing when you thought you saw a bug and it wasn't. 3. This is the reviewer's chance to stay uptodate with the codebase. 4. This is the reviewee's chance to share blame for mistakes. Nobody who reviewed can crap on them for a bug discovered later. People who couldn't bother to review can't complain either. 5. Make positive comments whenever you can honestly do so - just reduces the stress and fosters goodwill. 6. People who behave like arseholes in PRs are usually the kind you don't want in your team. i.e. it's a way of detecting such people - see how they use a bit of power.
- shepherdjerred 2y ago
  One thing I also like to do is send a Slack message or leave an extra comment on a PR with my overall feelings/praising anything I can. Also, the note about the formatter/linter is gold. I'm amazed at how many teams just live with constant nitpick comments on their PRs rather than automating these checks.
yinser 2y ago
The author and the reviewer universally want "good code" but how often is that defined for any project? Clear definitions of what is important to your team and your code base would alleviate a lot of these antipatterns.
BeetleB 2y ago
Here are some solutions I've seen in various teams: For one-off reviews, set the expectation: The developer is asking for feedback, and not permission. He doesn't need to justify to the reviewer why he didn't incorporate all the changes. Don't give people pointless authority over others. This will solve most of the problems in the submission. For more structured/important reviews, a third party moderator decides on disputed code changes. If you can't have this, then insist on > 1 reviewers and state you'll only make changes if the reviewers have consensus. This often solves style issues. Changes I would like to see, but have not seen: Reviewers should not ask questions - they should state concerns. So no "Why didn't you do this via X?". Instead, say "I think doing it via X instead of Y is better because ... ". And definitely banned: "Why did you solve it this way?"
- eyelidlessness 2y ago
  Asking questions is often the best part of review IME, at least when the overall review culture is healthy and productive. If nothing else, it’s often a good opportunity to add clarifying comments where something isn’t obvious after it was written. And authors receptive to good review, again IME, often agree that added clarity will help those cases. “Why did you solve it this way?” is also an excellent question in that context. Not for the big “overall approach”, where the common refrain is that design should come before implementation. But sometimes it helps surface smaller areas of complexity that only become obvious in the trenches, and it can be incredibly helpful to socialize those once discovered. (Also often prompting addition of clarifying comments.)
  - BeetleB 2y ago
    Asking questions is great if you're genuinely confused or need clarification. But if you have a concern, express it with the question so there is context. > “Why did you solve it this way?” "Because it solved the problem." Why are you asking me this question if you have no concerns? If you have some, why aren't you sharing them with me? If you're not going to, then I will give the minimally correct answer to your question. Conversations are a 2-way street. If you want me to spend a lot of time explaining it, then reciprocate! Anyone can ask open ended questions. It takes a lot more work to answer them than to ask. Make it worth my time to answer them! Forcing reviewers to put an effort in the discussions also solves a lot of problems in this submission. People aren't going to waste a lot of their own time playing these kinds of games. Make the barrier high enough to prevent 90% of bad actors from acting.
nemetroid 2y ago
Most of these, maybe all, have a dual pull request antipattern. > The Ransom Note A developer submits a patch that's especially important to them. When you point out that additional changes are needed elsewhere to make the component stay logically consistent, you get told that those changes are unrelated to the problem being solved (their problem being solved). > The Guessing Game A developer submits a substantial patch. They contents of the patch indicates that the submitter has not understood the design of the code they're modifying. When the reviewer communicating this back to the submitter, the submitter expects the reviewer to drop what they're working on and instead work on designing the feature the submitter needs. > The Priority Inversion A large patch has been submitted with poor explanation of the underlying design. Properly reviewing if the patch makes sense takes time. The submitter (publicly) complains that they (verifyably) aren't getting responses to their code reviews and that this is holding back feature X. > The Catch-22 There's a tradeoff to be made, and the decision made by the submitter is far off the mark. --- The problem usually isn't picky reviewers or sloppy submitters, it's the lack of clear contribution guidelines.
teqsun 2y ago
Sometimes the issues can come down to miscommunication, which I've found is well addressed by adopting a format for your PR comments to eliminate the ambiguity on what is blocking, what is an optional suggestion, what is a genuine question, etc., such as: https://conventionalcomments.org/ https://conventionalcomments.org/
hakunin 2y ago
This is a good "what not to do". A while ago I wrote a "what to do"[1], which could serve as a good complement to this article. [1]: https://max.engineer/mindful-code-reviews https://max.engineer/mindful-code-reviews
aidos 2y ago
For me there’s only one thing that matters - you’re all working together to put good code into the system. If you work in an environment where people lord code review over you in any way then you’re on a hiding to nowhere. That applies equally if you expect reviewers to not, you know, review your code. I see this discussion of “nitpicks” constantly. If it’s a tiny thing that brings the code in line with the existing codebase then just do it rather than fighting it. If there’s some ambiguity then find a rule as a team and apply it. I have no idea how people work in environments where you’re not all working together.
xpe 2y ago
Let's think about writing for a moment... Many forms of writing heavily benefit from having a clear goal to start, followed by an outline, followed by many iterations of the content. One analogous challenge with code reviews occurs when the reviewer can't quickly see something like a (a) goal and (b) outline. Without these two, it can make it hard to "navigate" what kind of feedback would be helpful. a. In many cases, the reviewer and reviewee might think they have the same goal, but this is often subtly or blaringly wrong. Even people in the same organization can serve different masters. b. Different software cultures have different ideas of planning, outlines, design documents, whatever. Some cultures wing it. Some people keep this kind of thing in their head, but it is hard to quickly share your head with someone else. Without some notion of the contours of the problem, how can you properly assess the the code?
distortionfield 2y ago
I see a lot of these comments boiling code review like this down to "picky reviewers" or "lazy submitters", but I have first-hand experience with something that doesn't fit either of these explanations, and it's when code review like this becomes used as a political weapon. In my case, another engineer that was gunning for my position as tech lead started to use several of the tactics listed in OP's post to delay and bog down my pull requests (where previously we had kept a good relationship) while they were getting their PRs rubber-stamped by a junior on our team while I was asleep (America vs. European hours). Management noticed and permanently soured on me and I was never able to recover from the damage done. I didn't fully realize what was happening until it was too late. I was laid off less than a month later and the other engineer was promoted. It was an extremely valuable lesson, though, and I got a better job inside of a month later, anyway, but it's something I keep a close eye on now.
deleted 2y ago
[deleted]
deschutes 2y ago
Trying to apply heuristics like patterns to this problem is attacking the wrong thing. It boils down to a lack of belonging, trust and motivation of reviewers to do the right thing. Absentee leadership not holding engineers accountable for their inaction or slow walking of reviews being the enabler. This kind of dysfunction happens more often on teams with incompetent people in control but sometimes temporarily happens on otherwise high functioning teams when multiple people are fighting over philosophical control of the project.
joshka 2y ago
I feel like I've probably seen both sides of all the anti-patterns listed and a few more to boot. A lot of these have some natural root causes. These apply pretty generally to both reviewer and reviewee. - not enough context (why is this change needed, why is the existing code this way) - not enough shared direction (we should do this thing, we've already decided we're not doing this thing) - misaligned expectations of time (review this 1000 line PR for me, I have a fulltime job) - hidden background (why can't we just... / we can't implement that because of this historical precedent) - different personality types / disorders (ASD and other things can mean nitpicks / technical issues are just as "important" as larger issues) - tooling problems (no way to defer submitting just the big things and hold back on the nits) - the code is a proxy for things which are part of the identity of the writer (tabs vs spaces) === The main things that I've seen that seem to help avoid these things are: 0. when possible establish some sort of rapport above and beyond just the review 1. assume positive intent. assume nothing else 2. communicate expectations / constraints 3. communicate appropriate background. 4. don't treat all communication the same, different reviewers sometimes need different approaches. 5. remember that the review is of the code, not the person (this applies to both parties)
notShabu 2y ago
behind a lot of these are power games made particularly perverse bc the participants tend to believe that they are above power games via dedication-to-the-truth or something the more knotted and unaccepted the actual hierarchy vs the perceived hierarchy, the more of this kind of behavior. the more people feel respected the less they have to force behavior from others to make up for the feeling of lack of respect. particularly toxic are places where a local tyrant believes that they deserve a lot more respect than they actually do (e.g. expert beginnerism)
skybrian 2y ago
I’d like to see a system where you don’t review patches, you fix them, and then you send it back and have them review the improved patch. The game ends when someone accepts the patch as-is. But maybe it would only work for peers on the same project.
jwindle47 2y ago
now I’m convinced that a certain senior engineer at a very well known tech company had this as an instruction manual. I’ve experienced each of these multiple times from the same person! On my own though, I’m certainly guilty of the 100 round trips. I tend to mentally group nits and not spell out every one that I find
solatic 2y ago
If you find yourself ping-ponging back and forth on the review, waiting 24h for a review that you fix in five minutes and then wait another 24h for re-review, you should treat it like any other asynchronous process that you find yourself wishing was synchronous - stop, get both you and the reviewer in a room, get to approval within the room, or at least to a point where it's clear you have more than five minutes worth of work to get to approval. Just because your code forge has a code review feature doesn't mean that all your code review behavior must be tracked in it.
nicolemarie6750 2y ago
[dead]

131 points by Tomte 2y ago

meowface 2y ago

It might be funny and educational to try to train an LLM to review code while following these antipatterns.

szundi 2y ago
Hilarious. Maybe easier than it seems at first: ask it to do it wrong for fun, i bet it would work
- zelphirkalt 2y ago
  "I cannot understand it in any other way than if you follow these anti-pattern. Please review this code."
Bjartr 2y ago
More constructively, train one to be able to notice them and intervene when they happen.

teqsun 2y ago

I still haven't encountered an environment where PRs aren't either essentially a rubber-stamping because an external audit said they needed to do code reviews, or the death-by-a-thousand-cuts as described in the article. I'm sure they exist in the working world, but 5 years in across 4 different companies I've yet to encounter it (beyond what I've tried to provide to others in my code reviews on their PRs).

ebiester 2y ago
So, there's a few things here: First, part of the benefit of peer review is that two people have seen the code and have familiarity. Even if it's rubber stamping, if someone has actually looked at it, LGTM might be enough. Second, even if PR only catches something 10% of the time, it catches the most obvious bugs.
occz 2y ago
I consider this document to be the golden standard for reviews, for both parties involved: https://github.com/google/eng-practices/blob/master/review%2Findex.md https://github.com/google/eng-practices/blob/master/review%2...
svaha1728 2y ago
Every now and then I can provide good feedback when I pull down the branch in question, test it locally, and try to think of a way to break it. Rarely do I have that kind of time.
- bojan 2y ago
  A code review is not about testing the functionality or finding bugs. It is about sanity checking the design (although in a welk performing team the design should never come as a surprise), complexity, readability, and test coverage.
bluGill 2y ago
When I was an - intern 30 years ago - the senior developer working with me had to decide if my code was worth a "formal review" or if just putting it in code review was enough. We went to code review, but as part of this I did hear about a process where code reviews are useful that apparently the company did use for important/difficult code. In a formal review you print off the code, then put 5-10 other developers in a meeting room to look at it together (no computers in this room, though sometimes you would go back and print off some more code for context). You could do about 10 lines of code per hour this way, and the developers burned our after at most 2 hours per day, so this takes a very long time. However reports were this resulted in the most useful reviews/fixes to the code. I've never seen the above done in practice, just heard about it. Someday I'd like to see it, but I doubt I ever will.
- nescioquid 2y ago
  That sounds like a Fagan review. I haven't been involved with one either, but I think some of the forces that drove people to do them have been weakened. When you are shipping code that you won't be able to update easily (i.e. cheaply), you try to remove as many defects as early in the process as possible. CI/CD and web delivery seem like they would blunt the motivation to go through a Fagan review for most software developed these days. I would imagine that some form of this still happens in safety-critical systems (or more likely the state of the art has advanced from this).
Phlebsy 2y ago
There are two ways I've found success where I've come into a team with these issues: 1. Differentiate critical comments vs opinions/nice to haves. Not every comment needs to be resolved as part of that specific PR, they can be addressed in a follow up after it is merged in a disciplined team. 2. Be okay with reviewers contributing to the PR themselves. Some developers just get too caught up on 'ownership' of code that makes it in, when the team is responsible for it in the end(when you have a blameless/improvement focused culture at least). I don't need the original branch owner to fix a typo or nitpicks when it will take me less time to fix it. I haven't found any code suggestion features that are amazing for this, so sometimes you'll end up with back/forth disagreement and have to rebase a commit out, but most of the time it's fine and overall improves velocity. Basically turning the PR into async pair programming.
- teqsun 2y ago
  Fair. I guess I've found that in my encounters, personally doing everything proper for other's code reviews hasn't inspired change in the other developers to reciprocate. i.e. I haven't walked into an environment with a lacking PR culture and managed to effect a cultural change (yet). Maybe seniority is a prerequisite?
- thehappyfellow 2y ago
  It’s big, particularly the second point. I fix typos and obvious mistakes while reading the code and I leave „feel free to punt” on any comment which shouldn’t block release. Still, bikeshedding occurs but to a lesser degree and over more substantial issues, like overall module design.
kagevf 2y ago
You can think of code reviews as an opportunity for knowledge transfer. ... and it has the added benefit that the person it's directed to will actually read your feedback since it would be directly related to work they're doing.

bluGill 2y ago

Doing a good code review is hard. You cannot do it in one pass, but of course projects are always late so there is pressure to approve. If you want code reviews to be good you need a policy that the reviewers need to approve it 3 days in a row without changes, but of course management wants to ship it and get on with the next feature. It is easy to find nits. Most people will notice a spelling error out of the corner of their eye and zoom in on it. (I'm a terrible speller and I do this all the time). It is much harder to notice that the code is wrong and has bugs. I've passed many code reviews with a few nits to fix and push, only the next day did I realize there were serious problems with the code.

gnulinux 2y ago
> projects are always late This is the root of all evil. The artificially created deadlines that make something that can be resolved next week "urgently today" just to increase your adrenaline levels so you work 30 mins more today is what causes a lot of software to end up being a piece of shit. Software is about deep thinking, and comprehensive understanding. Yes, it's all about trade offs, but there is a point where things can't be any faster.
- mikepurvis 2y ago
  I agree on one level, but I also know that I'm a person who needs to have some kind of external incentive or else I'll just mire myself in endless rewriting and refactoring; unclear whether that has to be a boss/customer wielding a deadline or if it's enough to have a colleague who leans slightly more to the other end of the practical-idealistic continuum, but I know my best work happens with at least some kind of pressure being applied.
  - ngcazz 2y ago
    I was recently diagnosed with ADD, and learning that about myself explained a behavior of mine which seems diametrically opposed to yours - the pressure of a deadline makes me panic, and somehow my outlet is bikeshedding. Give me daily and weekly commitments, and I'll do my most focused and highest quality work.

a1o 2y ago

This is great! I certainly can relate to a few of those!

pnathan 2y ago
I relate to them really well too!
jenbasasoy 2y ago
[flagged]

omoikane 2y ago

The death of a thousand round trips has an inverse where the reviewer points out everything they can find, and then the author fix just one thing. You might think the author is motivated to minimize round trips, but maybe they will pull the reverse ransom note next where they threatens to find a different reviewer or subvert the code review somehow, because "it's an emergency and the patch needs to go in now".

rwmj 2y ago
I get especially annoyed if I have to tell patch submitters to fix the same thing twice. Carefully read everything I wrote!
bityard 2y ago
This totally happened to me all the time in prior jobs with cultures of low quality and lack of ownership. When I review code, I'm always careful to prefix low-importance things with "[suggestion]" or "[nitpick]". These are small things like typos in comments that would not block me from approving the view. So people who know me know that I don't flag things frivolously. However, I would often review someone's code (someone I know and respect, mind you) with 3-5 changes that MUST be made before I would approve it. Because not fixing it would either be a bug, or would make life unnecessarily difficult later on. They would fix ONE of the things and I would have to point out that they didn't address any of the other things. And then they would go back and fix one more, and the cycle continues until they FINALLY get all the things addressed. Only once have I gotten worn down and approved substandard code because I was tired of all the back and forth. In that case, it was a student whose paid internship was almost up and he had to have it deployed before he left. (We never ended up using that project...)
- 5Qn8mNbc2FNCiVV 2y ago
  I 100% would have closed the MR with a note akin to "If you don't want to work you can just say so". That stuff doesn't fly at all in my teams, it's a disservice to everyone else it's insulting.

sharkjacobs 2y ago

> The Priority Inversion > In your first code review passes, pick small and simple nits. Variable names are a bit unclear; comments have typos in. > Wait for the developer to fix those, and then drop your bombshell: there’s a much more fundamental problem with the patch, that needs a chunk of it to be completely rewritten – which means throwing away a lot of the nitpick fixes you’ve already made the developer do in that part of the patch. I've done this more than once, and felt pretty bad about it each time. The problem is that I'm pretty good at spotting the little issues while I'm reading through some code. But I'm not as good at identifying the larger design issues until I'm thinking about it later

progbits 2y ago
I had a coworker who would leave nitpicks and bombshell in the same review round. Like 30 comments, 29 of which are irrelevant if you read the last one. Couldn't explain the problem to him... I just learned to read through full review from him first before starting to address anything. Otherwise he was one of the best people I worked with and the feedback was valuable.
- operator-name 2y ago
  In my mind it's important to have both, as long ad the main issue is made clear and nitpicks are truly nitpicks (and thus acceptable in limited quantity to not block a change). Otherwise you'd have a "bombshell" to refactoring, then on the 2nd review pass a handful of nitpicks that you could have addressed during the initial refactor.
- sqeaky 2y ago
  What really is the alternative? Just post the big bombshell thing and not express your opinion about stuff? Often it's the expressing opinion part that's more important than the actual nits.
  - progbits 2y ago
    It was like: > This whole thing won't work because of X and you need to pretty much rewrite all of this. > > Rename foo to bar. > Typo. > Missing whitespace. So yeah... Just don't post the rest? I often go back to earlier comment drafts and edit / delete them when I realize deeper into the review they are no longer relevant. The review comments should not be a stream of consciousness but properly written feedback.

kissgyorgy 2y ago

Code reviews can significantly sped up by pair programming a lot! When you wrote the code together, a lot of these doesn't even come up, because you discussed everything already. Sometimes after pair programming, even no code review is needed!

InfamousRece 2y ago
And if you need more than 1 reviewer just do (n+1)-tuple programming (where n is the number of reviewers needed). The more the merrier!
- shakezula 2y ago
  I think you're joking but there was a company in my area that famously stuck by their "mob programming" approach where 5+ developers would sit in a room, 1 would drive, and the rest would fill out an adjoining document.
mook 2y ago
Often I find reviews are useful precisely because of the distance the reviewer has to the code; a fresh set of eyes is good to catch issues. (Frequently I'd go over my own changes before submitting for review for the same reason). Pair/group programming also often ends up lacking in notes on what the thing was actually meant to be doing (because this was communicated verbally and never written down in reviews), which makes code archeology difficult. Or maybe that's just derived from the (external) code base I had to deal with that was done that way and doesn't generalize…

taeric 2y ago

I'm surprised there aren't more studies in how code going into the kernel is reviewed. There are various subsystems that have different levels of review. And it is largely done "in public" for the final set of changes. Heck, many of the tools we take for granted now were sharpened on the kernel.

AndyNemmity 2y ago

This is the one I get hit with constantly. The Ransom Note > This particular patch seems especially important to the developer who submitted it. (Maybe they said that outright, as part of their argument trying to persuade you to take the patch. Or maybe you just read between the lines.) > But this patch isn’t especially vital to you – so you’re in a position of power! Now you can hold the change they need hostage until they do lots of extra tangentially related work, which doesn’t really need to be in the same commit, but which is important to you. The reviewer outright denies this occurs, and it occurs every time. For a commit on changing the documentation, he will then want the entire documentation redone, including pages that aren't being changed. And will refuse to approve it until every issue he's brought up has been addressed. I've just stopped asking them for PR reviews, and ask other teams to review my PRs.

manmal 2y ago
I‘ve had a platform lead constantly try ransoming small refactorings into feature PRs, because he found out that’s the only way of achieving continuous architectural improvements. (Medical product, so the process was rigid)
- shepherdjerred 2y ago
  I can't tell if this is a complaint or admiration.
  - manmal 2y ago
    It was very annoying, but effective and kinda understandable, so yes, both :)
shepherdjerred 2y ago
I've been guilty of this too, and generally I'll write something along the lines of "this would be nice to do, but it's out of scope unless you want to take it on".
- 2y ago

zahlman 2y ago

Wow, that's a nostalgic domain name. I could have sworn I saw pieces by other authors on chiark.greenend.org.uk decades ago, perhaps even before the Internet became widely commercialized.

masfuerte 2y ago
It is an old domain. The registration date is "before Aug-1996", when Nominet was created to manage .uk domains.

xianshou 2y ago

In case you need a few more: The Nitpicker's Gambit: The reviewer fixates on trivial style issues like whitespace, bracket placement, variable naming conventions etc. They make the developer conform perfectly to their preferred style, even if it's not specified in the team's coding guidelines. The Silent Treatment: The reviewer provides no feedback at all for a long time after the review is requested, but does respond just often enough to keep the review "active". The author has to ping several times to get any response. The Tunnel Vision: The reviewer only looks at the specific lines changed, without considering the broader context of the code. They suggest changes that are locally valid but deliberately inconsistent with the overall design or architecture. The Ad Hominem: The reviewer makes snarky comments about the code in a way that implies the author is inexperienced and/or incompetent without directly saying so (and opening themselves to accusations of meanness). The Philosophical Debate: The reviewer gets into a long back-and-forth debate in the review comments about a matter of opinion like whether inheritance or composition is better. The actual issue at hand gets lost in the abstract discussion.

BeetleB 2y ago
> The Nitpicker's Gambit: The reviewer fixates on trivial style issues like whitespace, bracket placement, variable naming conventions etc. They make the developer conform perfectly to their preferred style, even if it's not specified in the team's coding guidelines. Mostly solved by autoformatters. If your team refuses to use one, start collecting metrics on time wasted in those discussions, and present to management. > The Silent Treatment: The reviewer provides no feedback at all for a long time after the review is requested, but does respond just often enough to keep the review "active". The author has to ping several times to get any response. Set a policy of auto-approve in N days if no engagement.

corytheboyd 2y ago

The commentary about “too big, make smaller!” vs “missing context, make bigger!” rings so true. It’s obviously a complex change either way, just pick one way and stick to it for that change. Not to be confused with the PR being “too big” because it is not atomic— that’s a different topic, something to teach the more junior teammates.

danielovichdk 2y ago

I think the list os fine but if those things happen in reviews a lot, I would argue you either have a cultural problem or simply have programmers that either don't pay attention or setup guardrails for eachother, for not submitting things that would get caught up in a review like this. Every decent IDE has an option for setting up some kind of configuration for how code should be named and structured . I think code reviews should be somewhat pedantic, it should be done with quality in mind and with the awareness of how the future changes. The list of patterns here are valid but if they tend to happen more than feels comfortable for the team, look at the culture of how you work.

teqsun 2y ago
A style-guide, linter, and formatter eliminates so many potential nits, but in my experience the teams that need them the most are often the most difficult/impossible to get any buy-in on.

xnorswap 2y ago

My favourite anti-pattern: Focus all your effort on catching errors that a linter and auto-formatter could catch. Shrug when questioned about architectural decisions, what really matters is that this field was PascalCase when it should be camelCase. When questioned why this isn't caught automatically by the build process, you have a few stock responses (Bonus: These may well be true!): - Linters are too slow, and a fast development experience is crucial - Linting the entire existing code base would effectively destroy "git blame" as a useful tool. - Different teams can't agree on whether to column-align wrapped function arguments, so they've agreed to disagree on this. So instead of automating away the mental overhead of checking for spaces vs tabs, or LF vs CRLF make sure you go over the PR with your own linter and nitpick every violation, even the suggestion level ones before you even consider trying to work out the validity of the submission.

nomel 2y ago
> Linters are too slow, and a fast development experience is crucial It's been, literally, 15 years since I've used an editor without linting built in. If someone tells me this, then it just means they don't gaf, and there will be bigger problems with their code.
sqeaky 2y ago
The most productive team I worked on just ignores this shit in code reviews. Like we will suggest stuff if the obvious formatting is wrong in a way the author probably didn't intend, but we're not going to hold up a code review on a nit. If there's one variable that's Pascal case instead of camel case or something that's not going to stop the code from going in if it's good code from algorithmic and documentation perspective. We just vaguely try to be consistent in each file and act like adults. We really agreed on value. We would insist that code that could be tested had tests. It was a C++ code base so we insisted that things be as fast as reasonable suggesting proper algorithms and data access patterns. And in general actually cared about bugs and making then shallow. Another point of agreement was that an automatic code formatter almost always made it look worse than having a few nits here and there. Anyone who mandates the things that really don't matter is just getting in the way.

t43562 2y ago

Another one I see is the insistent nitpicker who never accepts comments on their own PRs. FWIW: 1. Use a formatting tool and a linter in the build chain = zero formatting bullshit nitpicks. 2. Ask questions wherever possible rather than criticising. It's kinder and also less embarrassing when you thought you saw a bug and it wasn't. 3. This is the reviewer's chance to stay uptodate with the codebase. 4. This is the reviewee's chance to share blame for mistakes. Nobody who reviewed can crap on them for a bug discovered later. People who couldn't bother to review can't complain either. 5. Make positive comments whenever you can honestly do so - just reduces the stress and fosters goodwill. 6. People who behave like arseholes in PRs are usually the kind you don't want in your team. i.e. it's a way of detecting such people - see how they use a bit of power.

shepherdjerred 2y ago
One thing I also like to do is send a Slack message or leave an extra comment on a PR with my overall feelings/praising anything I can. Also, the note about the formatter/linter is gold. I'm amazed at how many teams just live with constant nitpick comments on their PRs rather than automating these checks.

yinser 2y ago

The author and the reviewer universally want "good code" but how often is that defined for any project? Clear definitions of what is important to your team and your code base would alleviate a lot of these antipatterns.

BeetleB 2y ago

Here are some solutions I've seen in various teams: For one-off reviews, set the expectation: The developer is asking for feedback, and not permission. He doesn't need to justify to the reviewer why he didn't incorporate all the changes. Don't give people pointless authority over others. This will solve most of the problems in the submission. For more structured/important reviews, a third party moderator decides on disputed code changes. If you can't have this, then insist on > 1 reviewers and state you'll only make changes if the reviewers have consensus. This often solves style issues. Changes I would like to see, but have not seen: Reviewers should not ask questions - they should state concerns. So no "Why didn't you do this via X?". Instead, say "I think doing it via X instead of Y is better because ... ". And definitely banned: "Why did you solve it this way?"

eyelidlessness 2y ago
Asking questions is often the best part of review IME, at least when the overall review culture is healthy and productive. If nothing else, it’s often a good opportunity to add clarifying comments where something isn’t obvious after it was written. And authors receptive to good review, again IME, often agree that added clarity will help those cases. “Why did you solve it this way?” is also an excellent question in that context. Not for the big “overall approach”, where the common refrain is that design should come before implementation. But sometimes it helps surface smaller areas of complexity that only become obvious in the trenches, and it can be incredibly helpful to socialize those once discovered. (Also often prompting addition of clarifying comments.)
- BeetleB 2y ago
  Asking questions is great if you're genuinely confused or need clarification. But if you have a concern, express it with the question so there is context. > “Why did you solve it this way?” "Because it solved the problem." Why are you asking me this question if you have no concerns? If you have some, why aren't you sharing them with me? If you're not going to, then I will give the minimally correct answer to your question. Conversations are a 2-way street. If you want me to spend a lot of time explaining it, then reciprocate! Anyone can ask open ended questions. It takes a lot more work to answer them than to ask. Make it worth my time to answer them! Forcing reviewers to put an effort in the discussions also solves a lot of problems in this submission. People aren't going to waste a lot of their own time playing these kinds of games. Make the barrier high enough to prevent 90% of bad actors from acting.

nemetroid 2y ago

Most of these, maybe all, have a dual pull request antipattern. > The Ransom Note A developer submits a patch that's especially important to them. When you point out that additional changes are needed elsewhere to make the component stay logically consistent, you get told that those changes are unrelated to the problem being solved (their problem being solved). > The Guessing Game A developer submits a substantial patch. They contents of the patch indicates that the submitter has not understood the design of the code they're modifying. When the reviewer communicating this back to the submitter, the submitter expects the reviewer to drop what they're working on and instead work on designing the feature the submitter needs. > The Priority Inversion A large patch has been submitted with poor explanation of the underlying design. Properly reviewing if the patch makes sense takes time. The submitter (publicly) complains that they (verifyably) aren't getting responses to their code reviews and that this is holding back feature X. > The Catch-22 There's a tradeoff to be made, and the decision made by the submitter is far off the mark. --- The problem usually isn't picky reviewers or sloppy submitters, it's the lack of clear contribution guidelines.

teqsun 2y ago

Sometimes the issues can come down to miscommunication, which I've found is well addressed by adopting a format for your PR comments to eliminate the ambiguity on what is blocking, what is an optional suggestion, what is a genuine question, etc., such as: https://conventionalcomments.org/ https://conventionalcomments.org/

hakunin 2y ago

This is a good "what not to do". A while ago I wrote a "what to do"[1], which could serve as a good complement to this article. [1]: https://max.engineer/mindful-code-reviews https://max.engineer/mindful-code-reviews

aidos 2y ago

For me there’s only one thing that matters - you’re all working together to put good code into the system. If you work in an environment where people lord code review over you in any way then you’re on a hiding to nowhere. That applies equally if you expect reviewers to not, you know, review your code. I see this discussion of “nitpicks” constantly. If it’s a tiny thing that brings the code in line with the existing codebase then just do it rather than fighting it. If there’s some ambiguity then find a rule as a team and apply it. I have no idea how people work in environments where you’re not all working together.

xpe 2y ago

Let's think about writing for a moment... Many forms of writing heavily benefit from having a clear goal to start, followed by an outline, followed by many iterations of the content. One analogous challenge with code reviews occurs when the reviewer can't quickly see something like a (a) goal and (b) outline. Without these two, it can make it hard to "navigate" what kind of feedback would be helpful. a. In many cases, the reviewer and reviewee might think they have the same goal, but this is often subtly or blaringly wrong. Even people in the same organization can serve different masters. b. Different software cultures have different ideas of planning, outlines, design documents, whatever. Some cultures wing it. Some people keep this kind of thing in their head, but it is hard to quickly share your head with someone else. Without some notion of the contours of the problem, how can you properly assess the the code?

distortionfield 2y ago

I see a lot of these comments boiling code review like this down to "picky reviewers" or "lazy submitters", but I have first-hand experience with something that doesn't fit either of these explanations, and it's when code review like this becomes used as a political weapon. In my case, another engineer that was gunning for my position as tech lead started to use several of the tactics listed in OP's post to delay and bog down my pull requests (where previously we had kept a good relationship) while they were getting their PRs rubber-stamped by a junior on our team while I was asleep (America vs. European hours). Management noticed and permanently soured on me and I was never able to recover from the damage done. I didn't fully realize what was happening until it was too late. I was laid off less than a month later and the other engineer was promoted. It was an extremely valuable lesson, though, and I got a better job inside of a month later, anyway, but it's something I keep a close eye on now.

deleted 2y ago

[deleted]

deschutes 2y ago

Trying to apply heuristics like patterns to this problem is attacking the wrong thing. It boils down to a lack of belonging, trust and motivation of reviewers to do the right thing. Absentee leadership not holding engineers accountable for their inaction or slow walking of reviews being the enabler. This kind of dysfunction happens more often on teams with incompetent people in control but sometimes temporarily happens on otherwise high functioning teams when multiple people are fighting over philosophical control of the project.

joshka 2y ago

I feel like I've probably seen both sides of all the anti-patterns listed and a few more to boot. A lot of these have some natural root causes. These apply pretty generally to both reviewer and reviewee. - not enough context (why is this change needed, why is the existing code this way) - not enough shared direction (we should do this thing, we've already decided we're not doing this thing) - misaligned expectations of time (review this 1000 line PR for me, I have a fulltime job) - hidden background (why can't we just... / we can't implement that because of this historical precedent) - different personality types / disorders (ASD and other things can mean nitpicks / technical issues are just as "important" as larger issues) - tooling problems (no way to defer submitting just the big things and hold back on the nits) - the code is a proxy for things which are part of the identity of the writer (tabs vs spaces) === The main things that I've seen that seem to help avoid these things are: 0. when possible establish some sort of rapport above and beyond just the review 1. assume positive intent. assume nothing else 2. communicate expectations / constraints 3. communicate appropriate background. 4. don't treat all communication the same, different reviewers sometimes need different approaches. 5. remember that the review is of the code, not the person (this applies to both parties)

notShabu 2y ago

behind a lot of these are power games made particularly perverse bc the participants tend to believe that they are above power games via dedication-to-the-truth or something the more knotted and unaccepted the actual hierarchy vs the perceived hierarchy, the more of this kind of behavior. the more people feel respected the less they have to force behavior from others to make up for the feeling of lack of respect. particularly toxic are places where a local tyrant believes that they deserve a lot more respect than they actually do (e.g. expert beginnerism)

skybrian 2y ago

I’d like to see a system where you don’t review patches, you fix them, and then you send it back and have them review the improved patch. The game ends when someone accepts the patch as-is. But maybe it would only work for peers on the same project.

jwindle47 2y ago

now I’m convinced that a certain senior engineer at a very well known tech company had this as an instruction manual. I’ve experienced each of these multiple times from the same person! On my own though, I’m certainly guilty of the 100 round trips. I tend to mentally group nits and not spell out every one that I find

solatic 2y ago

If you find yourself ping-ponging back and forth on the review, waiting 24h for a review that you fix in five minutes and then wait another 24h for re-review, you should treat it like any other asynchronous process that you find yourself wishing was synchronous - stop, get both you and the reviewer in a room, get to approval within the room, or at least to a point where it's clear you have more than five minutes worth of work to get to approval. Just because your code forge has a code review feature doesn't mean that all your code review behavior must be tracked in it.

nicolemarie6750 2y ago

[dead]