12 ms·

Mistakes I see engineers making in their code reviews

167 points by zdw 1y ago

watwut 1y ago
This is actually good article.
makeitdouble 1y ago
In agreement with most of the points, this one was surprising to me: > Many engineers seem to think it’s rude to leave a blocking review even if they see big problems, so they instead just leave comments describing the problems. Don’t do this. [...] Just leaving comments should mean “I’m happy for you to merge if someone else approves, even if you ignore my comments.” Do people actually ignore a comment explaining a problem with the code as written just because it wasn't a blocking review ? It's like ignoring someone telling you you're stepping into a hole because they're not grabbing you by the neck. Reviews shouldn't be that adversarial nor hand holding. I'm also realizing, everywhere I work a comment is basically a blocking, except if it's explicitely flagged or discussed as optional. Trying to find some other person to review and ignore the comment is just a big NO.
- sevenseacat 1y ago
  > Do people actually ignore a comment explaining a problem with the code as written just because it wasn't a blocking review ? Yes, yes they do. And some devs will go the other way - not matter how minor a comment you leave, even just those that are tagged nitpick/FYI, they must address them. I'm in agreement with most of the points, but I still find that most of the PRs I review, I block and request changes. Maybe that says more about me and the devs I've worked with...
  - hinkley 1y ago
    I can't decide if github is better or worse for making you 'resolve' all conversation whether you choose to do anything about them or not.
- BugsJustFindMe 1y ago
  > Do people actually ignore a comment explaining a problem with the code as written just because it wasn't a blocking review ? Approval is approval. If you complete your review and don't block, you have concretely indicated to the author that in fact nothing is wrong with the diff and it is ok to be merged as is no matter what else you said. Many authors won't even look at comments if their change is approved.
  - JoeAltmaier 1y ago
    That's a bit unprofessional. The comment may be about a non-blocking issue. There is something wrong with the diff, and you should look into it. Ignoring the comments is a tactic of a careless coworker. The diff may get merged and they can move on, sure. But come review time, they may find something else in their work environment is being rejected and removed.
    - siva7 1y ago
      On the contrary, parents comment is to me the professional variant if there is no other agreement. Every modern Code Management Platform has a distinct feature to mark a review as blocking or non-blocking, so it should be understood and communicated by the team how to use this feature.
      - JoeAltmaier 1y ago
        And it should be understood that if a coworker has something to say about your code, you should listen. Anything else is obstructive and arrogant. Which are both unprofessional behaviors.
        BugsJustFindMe 1y ago
        It's far more arrogant to expect/demand changes without blocking than it is for the author to take the reviewer at their word about whether the change needs additional work or not. By leaving a non-blocking review, the coworker has said that they have no strong objection to merging the code as is. If it makes you feel better, just think of ignoring non-blocking comments as equivalent to reading them and disagreeing.
- watwut 1y ago
  > Do people actually ignore a comment explaining a problem with the code as written just because it wasn't a blocking review ? Some people leave flood of comments that cosplay as explanation, but they are not one. Or I simply disgree. I am just getting to the point of ignoring them, because the alternative is to write an essay over each of those comments just for it to be ignored and the person passive aggressively ignoring the patch forever.
  - makeitdouble 1y ago
    I feel for you, I'd do whatever in my power to get out of that kind of toxicity.
- papanoah 1y ago
  Personally I hate it when people just leave comments on my PR without explicitly blocking or approving it. To me it comes across as indecisive. Either you are fine with the changes -> approve. If you think this code shouldn't be merged in it's current state -> block. Just leaving a comment feels like you want to complain, but don't really take any responsibility for what happens next. There are exceptions of course and it all depends on the comments and the circumstances, but I generally prefer explicit yes or no.
  - rkomorn 1y ago
    The way I look at it, commenting without approval means "I don't approve (and here's why) but someone else can." Blocking means "I don't approve and no one else should either."
    - dakiol 1y ago
      > I don't approve (and here's why) but someone else can That just sucks... because with that mindset typically nobody approves and leaves the submitter begging for approvals.
      - rkomorn 1y ago
        This does not improve with someone blocking the PR.
        papanoah 1y ago
        It sends a different message, in my opinion. Blocking means "I disagree, but lets figure it out and work together to get it over the finish line". "I don't approve, but someone else can" is very non-commital. Which gives me the feeling of being left alone with a bunch of critique, without appreciation for the work that I have originally done. I would wish my reviewer takes responsibility for his/her feedback. "I don't approve, but someone else can" also means to me "Merge it, if you must. If it works out, good for you, I havent blocked it. If it doesnt work out, I get to say 'See, I told you so!'.
- deleted 1y ago
  [deleted]
- sidewndr46 1y ago
  > Do people actually ignore a comment explaining a problem with the code as written just because it wasn't a blocking review ? I have worked with individuals who would assign NULL to a pointer, then immediately dereference it. I would bring up that this obviously would not do anything useful. The response I would get was that they already had the code working in their dev. environment, so there could not be any bugs.
donatj 1y ago
I work with a person where I know going into a review that I have to check every single file path. They have this weird ability to consistently put things in entirely the wrong folder/namespace. For example, something related to Categories might end up under Orders somehow... it's like new files just go in whatever folder they happen to have open at the time. I frankly don't even know they do it so consistently. Something to keep in mind in your reviews as well I guess, lol.
hexbin010 1y ago
I'd also add: check out the branch locally. Your colleagues do some awful things to their local environments, and the central CI isn't perfect. Your IDE might have more checks enabled than both, too I've worked in teams where I'm the only one who actually checks out the branch to poke around
- siva7 1y ago
  I strongly disagree on this one. The CI pipeline has to be the single source of truth. Your approach is a step back to the times of "but it works on my machine".
  - hexbin010 1y ago
    Yes, ideally. Then again, I've caught plenty of bugs checking it out and running it locally. The CI pipeline may not wholly be in your team's control. I've worked at places where a 'devops' team had too much control, liked to tinker all the time and often broke stuff. Nothing we could do - devops were gods and didn't take input from anyone. Should I not do check it out locally and instead spend months and political capital arguing for more in the CI (which the PM would argue slows down velocity - and will win)? Pragmatism and workarounds are often employed in non-bigtech enterprise companies... edit: I also meant 'poke around' as in do a bit of informal QA. Again, your test and QA team might not be perfect...
  - nabbed 1y ago
    There are times where it is reasonable, especially for bug fixes where there is a test to explicitly check that the bug is fixed. In that case, I might check out the code, revert the fix, and ensure that the test fails. There have been a few times I did that and the test still succeeded, so I can confirm that the test is not testing the right thing (in a very mature project, complexity or test settings can obscure what your test is actually doing, especially if it is an E2E test).
    - hexbin010 1y ago
      Yup, same here
jaxelr 1y ago
> Don’t leave too many comments I'll say this is directly proportionate to the size of the changes. If a code review is a conversation between n parties, it seems reasonable to me that more code leads to more comments.
siva7 1y ago
Good Points. The most important consideration from my experience having worked with more than 40 teams: "Code review is not the time for you to impose your personal taste on a colleague." There are always many acceptable ways how to solve a problem in software development, so if someone imposes his personal preference over that from a colleague this creates tension and a time sink for the team - don't make this a blocking review! This also plays a big role when developers obsess over code but lack the vision and experience to see that not everything will be a relevant factor for technical debt in the grand scheme for the business.
- rileymat2 1y ago
  There is a major exception to the personal tastes issue, consistency and predictably in a mature codebase. Things that would be personal taste in a greenfield project become important for maintenance in a mature product.
  - watwut 1y ago
    Then make that explicit topic during the setup instead of taking random commits hostage to whatever catches your attention this week.
    - rileymat2 1y ago
      It is hard to fully specify all the different dimensions of consistency. Look at the code that exists and write in that voice. Maintenance will be much better.
      - watwut 1y ago
        It is even harder to chase personal consistency feelings of multiple different colleagues. Usually inconsistent with each other. Usually enforced only against less assertive committers and not enforced against those perceived to be strong. That is why you should either make it a rule or let it be.
      - hinkley 1y ago
moltar 1y ago
Is anyone else using conventional comments https://conventionalcomments.org/ https://conventionalcomments.org/
- shagie 1y ago
  I'm a proponent of them. They help separate out atomic commits ("I'm writing the feature here... I'm doing a refactor there"). They also serve for being able to quickly identify where I need to look in the story I told myself for "what did I do there?" While the goals that it is designed to get people to follow is something that I've preferred in my own commits in the past, this gives a structure to them that helps identify when there's too much in a commit for it to be reasonably reviewed in isolation.
deleted 1y ago
[deleted]
awithrow 1y ago
I think a lot of this stems from the code review tools themselves. Especially the "Don't just review the diff" mistake. Especially with GitHub defaulting to just showing a list of diffs and changed files. I'd find it much more useful if a review tool started with a class hierarchy or similar high level view to get a sense of: 1. What/how many classes changed? 2. What/how many methods have been added/removed/modified 3. What method signatures have changed 4. What changes are covered by new tests "Don't leave too many comments" i think can really be rethought of, don't review style and syntax. Leave that to the robots. If you're relying on other engineers to flag style problems and linting, you're just wasting everybody's time. Set up linting and style checkers and be done with it.
- hinkley 1y ago
  A martial arts instructor does not tell you everything you're doing wrong on the first day. You would quit. And justifiably so. What an asshole. You know you've progressed in their eyes every time they start bugging you about something new. You didn't suddenly become worse at something. Rather you got good enough on some higher priority thing that they knocked it off the list and replaced it with the next item in the backlog. You should treat code reviews similarly. It's a journey, and we are in the middle. If you keep making the same comments on reviews, eventually they'll get addressed beforehand and you can point out something else.
dchuk 1y ago
I feel like a useful tool someone should build now that LLMs are so capable, is some sort of automated walk through of a pull request, where it steps a reviewer through initially the overview and they why of the changes, then step by step through each change, with commentary along the way generated by the LLM (and hopefully reviewed by the pull requester for accuracy). Then the reviewer can leave comments along the way. I’ve always found the cognitive overhead of going through a pull request challenging, seems like the paradigm can shift now with all of the new tooling available to all of us.
- clickety_clack 1y ago
  It seems like maybe you should just talk to your colleague who wrote the code. It would be far more efficient than having them create and review a whole review experience on top of the work that they did, and then have you try to understand a walkthrough document (that might have hallucinated explanations that the requester didn’t quite catch) plus the underlying code, plus still have to talk to them to ask clarifying questions anyway.
  - thunky 1y ago
    This sounds like you're making an argument against pull request descriptions in general. Like you just want to see code changes and have a meeting about it.
    - 1718627440 1y ago
      You can have both sync and aync communication. You can also talk to people without scheduling a meeting.
      - thunky 1y ago
        > You can have both sync and aync communication Sure, and PR descriptions and comments are a way to do that (async). But IME it helps to start with a good description of what the changes are rather than just having a pile of code changes dumped on you where you then have to reverse engineer the intent. It seems draining to me to have to have to start from nothing and interrogate someone for every PR that comes in. > You can also talk to people without scheduling a meeting I didn't say anything about scheduling.
OptionOfT 1y ago
I'm a little stricter. In terms of formatting etc I rely on tooling which break the CI. That way they don't even surface. Equally if something slips through the CI needs to be updated. A piece of code should match the general style of the code it is being integrated into. When you create an addition to a wood-framed house the addition won't be steel construction, unless you have an explicit reason (weight, speed, foundation etc). And if I'm the main maintainer of a codebase, it is something I can call out on. If we use Tokio and you use raw threads I'll ask about this choice. In an ideal world you (the PR author) includes this in its description, because it is a reflection of your understanding of the codebase. Reviewing code with 'will this work' is also debatable. Someone might propose a change that fixes an issue they are having. But maybe it's not something we want to solve in our codebase, or maybe it breaks other things the maintainers are thinking off.
- maleldil 1y ago
  > In terms of formatting etc I rely on tooling which break the CI. That way they don't even surface. Equally if something slips through the CI needs to be updated I also do something similar with lint rules. For Rust, this means turning on every single reasonable clippy lint (there are many), and making all warnings into errors. Some would say that some rules are overzealous or annoying, but it's a price I'm willing to pay to keep the codebase uniform.
  - OptionOfT 1y ago
    Same here, I have a Cargo.toml in which I painstakingly went through every lint and decided whether it made sense to me.
- hinkley 1y ago
  I contributed to an OSS project recently that ran the linter before the tests and it absolutely trashed my code-build-test cycle. If I felt less strongly about the value of the project I would have bailed. If I’m trying to get the code to work don’t cockblock me on Make it Work with your Make it Right enforcements.
AndyNemmity 1y ago
The thing that always bothers me is when the comments involve things that have nothing to do with the PR. Like, they are reasonable ideas, but open up a new issue. If every reviewer wants to tackle large topics in the PR Review that have nothing to do with what specifically is happening, then it explodes and gets even harder for others to review now that we are changing things that have nothing to do with the change.
- hinkley 1y ago
  It can be tricky to make a change in a file that is considered to be rotten. Especially for high priority tickets. Yes, we want to clear all of this code up. But you have to be very careful not to punish people for trying. I had to tell one guy to knock it off because I was the only person brave enough to touch certain files and he was quickly making me regret trying by marking blocking comments on things I already planned to address in the subsequent or following PR. But I have to keep the old and new stuff working at the same time, boyo, so tap the brakes.
dakiol 1y ago
It's not that complicated: - you review and if to the best of your knowledge you think something can be done better you comment about it and leave a suggestion on how to do it better - then you approve the PR. Because your job is not to gatekeep the code
- arccy 1y ago
  words by someone who has never needed to be responsible for running code. no wonder people like ai vibecoding.
  - jakewins 1y ago
    There is a middle ground here though. I try, anymore, to make sure I’m very clear on which review comments are blockers for me and which - most - are style, suggestions, questions. Unless the code has some egregious problem I immediately approve it and write a comment saying “this is good by me if you fix blockers A, B”; that way they are not then blocked by me doing a second review unless they want it. Maybe this is because I’m working somewhere where we don’t use stacked reviews though? So it’s a major pain for someone to have a PR open for a long time going through lots of cycles of review, because it’s tricky to build more work on top of the in-review work
    - arccy 1y ago
      i guess there's also a difference in the places you work at. the places i work at expect trunk to always be clean, and ready for production (continuous delivery). if you work someplace with a slower release cycle, then getting a not perfect change in may be more acceptable. there's also responsibility, which is traced during incidents back to author and reviewers. i won't approve until i'm confident in the code, and that will mean the author needs to answer questions etc.
  - hinkley 1y ago
    Some people don't know what responsible means. To some people, "I take full responsibility," means, "I will publicly admit to being wrong," instead of, "I will do everything in my power to clean up this mess that is my creation."
Learner100 1y ago
> When you receive a review with a hundred comments, it’s very hard to engage with that review on anything other than a trivial level. Wow, that seems crazy. I can only hope I never have to work with somebody who thinks it is productive to leave that many comments on a change -- I genuinely cannot imagine any change that could ever require that. Great article, fully agree with all the points.
- arccy 1y ago
  usually it means your pr is too big and/or you didn't discuss it enough beforehand...
  - Learner100 1y ago
    In those situations, the more productive option to course-correct is to talk to the change author in a meeting/chat instead of just volleying off a tsunami of comments about various minutiae in the change IMO.
- 5Qn8mNbc2FNCiVV 1y ago
  I've had some PRs that would have required a ton of comments. There are usually two ways I handle this: If the PR went in a completely different direction and missed the goal by a lot, I take ownership of it (with a brief explanation) and re-implement it. I then use the new PR for a pairing session, where I walk through both PRs with the original author for learning purposes. If it’s mostly smaller issues, I schedule a half-hour pairing session with the author and review everything together, after preparing a list of issues. Doing it any other way puts too much burden on the author to guess what the reviewer wants, and it slows down velocity significantly.
- hinkley 1y ago
  When the PR system is being used as a weapon to freeze someone out, everything has gone to shit.
suralind 1y ago
I disagree with the point „don’t leave too many comments”. I think it’s easier to read a code that is a) following repo standards, b) following community standards. If a PR requires a 100 comments, so be it. Granted, I also almost always give a benefit of the doubt to the author unless a change is really required and I only ever seen a PR with close to a 100 comments once or twice. I should also say that you shouldn’t leave n comments with the same content so usually there’s no need for more than a couple comments in total.
cadamsdotcom 1y ago
There is opportunity in the meta. If you are leaving a comment it’s because someone didn’t learn how things should be from the tools in your codebase. Naming style problem? Linting isn’t catching it. Architecture violation? Your import linting (checkers to prevent route handlers importing the DB and skipping the service layer, etc.) isn’t catching it. Duplicate method? What tooling can you create in the LLM era - remembering you can ask agents to build scripts now?
criemen 1y ago
Great article, and I mostly agree. I want to nitpick on > Don’t review with a “how would I write it?” filter _Thinking_ about "how would I write it?" when reviewing code catches a lot of stuff for me - I want to be able to come up with how I'd do it, see how the PR does it, and then check if the approach matches. If not, there's something to be learned and explained here. Although, all of that is in my head! If I come to the conclusion that the presented approach is wrong or inferior in an objective way (say, doesn't handle corner cases neatly or at all), I'll comment. Otherwise, let people do their thing! So, I agree with the point of not using this to generate a lot of comments, but I do believe in it as a useful technique to engage the brain, instead of just loosely scrolling over the diff.
- hinkley 1y ago
  A navigable code base is one where people can either 1) guess where a particular bit of functionality should be implemented, 2) find a paper trail that leads them to the spot where it is. If you can't find it, you can't fix bugs. If you don't find it, then you end up splitting related functionality across several places in the code. Which will introduce bugs, and make them a pain in the ass to diagnose.
ramanvarma 1y ago
I'm not convinced code review should be about "consistency" or "system memory" at all. Half of the dysfunction in large codebases comes from reviewers over-optimizing for long-term purity instead of short-term progress.
mock-possum 1y ago
Code review is the job, in a lot of ways - whether it’s code you wrote, code a coworker wrote, or code generated by an LLM - you are asked “is it okay if this code joins the codebase?) It’s no small responsibility.
kiliancs 1y ago
I see where this sort of take comes from. Reviews are not a place for subjective comments that just slow down or block progress, signaling, excessive nitpicking, etc. However, the reviewer should be thinking beyond the immediate "hey, this is progress", and also consider how this change affects the codebase and how it aligns with feature and technical direction. A few happy approvals and a great codebase becomes hard to work with, if contributors are inexperienced. Code reviews are also an opportunity for learning, and provide real-life scenarios to inform decisions that are otherwise not grounded and mostly opinion. Of course, not all comments and discussions should block approval, but there is a balance. No one is going to die if this feature is delayed by a few days if you get a better result _and_ are investing in team growth.
3abiton 1y ago
> Many software engineers now spend as much (or more) time reviewing the output of their own AI tools than their colleagues’ code. This hits home.
deleted 1y ago
[deleted]
hinkley 1y ago
> If tons of PRs are being blocked, it’s usually a sign that there’s too much gatekeeping going on. I would expand this and say if too many of the blocking statuses are coming from one reviewer, too much gatekeeping may be going on, even if the overall rate is low. Either they're going too far, or nobody else is going far enough. If you're shouting into the void, discretion is the better part of valor. You can't save people who don't want to be saved. Find people who will appreciate your efforts instead of resenting them.
hinkley 1y ago
> For many engineers - including me - it feels good to leave a blocking review, for the same reasons that it feels good to gatekeep in general. Clearly this is not true for everyone, but I don't think I've ever felt good about gatekeeping, and a number of people I respect generally don't either. Blocking is a weapon of last resort. That someone has had to use it means either that the system isn't working, or they're abusing the tool. Someone shouldn't arrive at the final phase of their ticket and be told to start it over. That's a retrospective item, and if it's happened multiple times then this is an agenda item at the retro meeting.
- rkomorn 1y ago
  > Blocking is a weapon of last resort This is exactly how I feel. The only times I would block a PR is if I know it will either: a- actually break something existing b- not deliver what the PR says it should deliver (eg new feature but the feature doesn't actually work) It's basically saying "I'm certain that if we merge this, something bad will happen and we'll either cause an incident or need a hotfix".
  - hinkley 1y ago
    c - it represents a complete failure to adhere to a standard of coding that we all approved as a group. Any of these things are a negative commentary on your process, or the members of your team. When one finds oneself standing in a hole, it is important to stop digging.
    - rkomorn 1y ago
      Agreed on c- . The rest seems a bit strident. Stuff happens. I wouldn't exactly go running to the incident/retro unless there was repeated pattern.
blks 1y ago
If I see that majority of code is generated, I reject the PR

167 points by zdw 1y ago

This is actually good article.

In agreement with most of the points, this one was surprising to me: > Many engineers seem to think it’s rude to leave a blocking review even if they see big problems, so they instead just leave comments describing the problems. Don’t do this. [...] Just leaving comments should mean “I’m happy for you to merge if someone else approves, even if you ignore my comments.” Do people actually ignore a comment explaining a problem with the code as written just because it wasn't a blocking review ? It's like ignoring someone telling you you're stepping into a hole because they're not grabbing you by the neck. Reviews shouldn't be that adversarial nor hand holding. I'm also realizing, everywhere I work a comment is basically a blocking, except if it's explicitely flagged or discussed as optional. Trying to find some other person to review and ignore the comment is just a big NO.

sevenseacat 1y ago
> Do people actually ignore a comment explaining a problem with the code as written just because it wasn't a blocking review ? Yes, yes they do. And some devs will go the other way - not matter how minor a comment you leave, even just those that are tagged nitpick/FYI, they must address them. I'm in agreement with most of the points, but I still find that most of the PRs I review, I block and request changes. Maybe that says more about me and the devs I've worked with...
- hinkley 1y ago
  I can't decide if github is better or worse for making you 'resolve' all conversation whether you choose to do anything about them or not.
BugsJustFindMe 1y ago
> Do people actually ignore a comment explaining a problem with the code as written just because it wasn't a blocking review ? Approval is approval. If you complete your review and don't block, you have concretely indicated to the author that in fact nothing is wrong with the diff and it is ok to be merged as is no matter what else you said. Many authors won't even look at comments if their change is approved.
- JoeAltmaier 1y ago
  That's a bit unprofessional. The comment may be about a non-blocking issue. There is something wrong with the diff, and you should look into it. Ignoring the comments is a tactic of a careless coworker. The diff may get merged and they can move on, sure. But come review time, they may find something else in their work environment is being rejected and removed.
  - siva7 1y ago
    On the contrary, parents comment is to me the professional variant if there is no other agreement. Every modern Code Management Platform has a distinct feature to mark a review as blocking or non-blocking, so it should be understood and communicated by the team how to use this feature.
    - JoeAltmaier 1y ago
      And it should be understood that if a coworker has something to say about your code, you should listen. Anything else is obstructive and arrogant. Which are both unprofessional behaviors.
      - BugsJustFindMe 1y ago
        It's far more arrogant to expect/demand changes without blocking than it is for the author to take the reviewer at their word about whether the change needs additional work or not. By leaving a non-blocking review, the coworker has said that they have no strong objection to merging the code as is. If it makes you feel better, just think of ignoring non-blocking comments as equivalent to reading them and disagreeing.
watwut 1y ago
> Do people actually ignore a comment explaining a problem with the code as written just because it wasn't a blocking review ? Some people leave flood of comments that cosplay as explanation, but they are not one. Or I simply disgree. I am just getting to the point of ignoring them, because the alternative is to write an essay over each of those comments just for it to be ignored and the person passive aggressively ignoring the patch forever.
- makeitdouble 1y ago
  I feel for you, I'd do whatever in my power to get out of that kind of toxicity.
papanoah 1y ago
Personally I hate it when people just leave comments on my PR without explicitly blocking or approving it. To me it comes across as indecisive. Either you are fine with the changes -> approve. If you think this code shouldn't be merged in it's current state -> block. Just leaving a comment feels like you want to complain, but don't really take any responsibility for what happens next. There are exceptions of course and it all depends on the comments and the circumstances, but I generally prefer explicit yes or no.
- rkomorn 1y ago
  The way I look at it, commenting without approval means "I don't approve (and here's why) but someone else can." Blocking means "I don't approve and no one else should either."
  - dakiol 1y ago
    > I don't approve (and here's why) but someone else can That just sucks... because with that mindset typically nobody approves and leaves the submitter begging for approvals.
    - rkomorn 1y ago
      This does not improve with someone blocking the PR.
      - papanoah 1y ago
        It sends a different message, in my opinion. Blocking means "I disagree, but lets figure it out and work together to get it over the finish line". "I don't approve, but someone else can" is very non-commital. Which gives me the feeling of being left alone with a bunch of critique, without appreciation for the work that I have originally done. I would wish my reviewer takes responsibility for his/her feedback. "I don't approve, but someone else can" also means to me "Merge it, if you must. If it works out, good for you, I havent blocked it. If it doesnt work out, I get to say 'See, I told you so!'.
deleted 1y ago
[deleted]
sidewndr46 1y ago
> Do people actually ignore a comment explaining a problem with the code as written just because it wasn't a blocking review ? I have worked with individuals who would assign NULL to a pointer, then immediately dereference it. I would bring up that this obviously would not do anything useful. The response I would get was that they already had the code working in their dev. environment, so there could not be any bugs.

donatj 1y ago

I work with a person where I know going into a review that I have to check every single file path. They have this weird ability to consistently put things in entirely the wrong folder/namespace. For example, something related to Categories might end up under Orders somehow... it's like new files just go in whatever folder they happen to have open at the time. I frankly don't even know they do it so consistently. Something to keep in mind in your reviews as well I guess, lol.

hexbin010 1y ago

I'd also add: check out the branch locally. Your colleagues do some awful things to their local environments, and the central CI isn't perfect. Your IDE might have more checks enabled than both, too I've worked in teams where I'm the only one who actually checks out the branch to poke around

siva7 1y ago
I strongly disagree on this one. The CI pipeline has to be the single source of truth. Your approach is a step back to the times of "but it works on my machine".
- hexbin010 1y ago
  Yes, ideally. Then again, I've caught plenty of bugs checking it out and running it locally. The CI pipeline may not wholly be in your team's control. I've worked at places where a 'devops' team had too much control, liked to tinker all the time and often broke stuff. Nothing we could do - devops were gods and didn't take input from anyone. Should I not do check it out locally and instead spend months and political capital arguing for more in the CI (which the PM would argue slows down velocity - and will win)? Pragmatism and workarounds are often employed in non-bigtech enterprise companies... edit: I also meant 'poke around' as in do a bit of informal QA. Again, your test and QA team might not be perfect...
- nabbed 1y ago
  There are times where it is reasonable, especially for bug fixes where there is a test to explicitly check that the bug is fixed. In that case, I might check out the code, revert the fix, and ensure that the test fails. There have been a few times I did that and the test still succeeded, so I can confirm that the test is not testing the right thing (in a very mature project, complexity or test settings can obscure what your test is actually doing, especially if it is an E2E test).
  - hexbin010 1y ago
    Yup, same here

jaxelr 1y ago

> Don’t leave too many comments I'll say this is directly proportionate to the size of the changes. If a code review is a conversation between n parties, it seems reasonable to me that more code leads to more comments.

siva7 1y ago

Good Points. The most important consideration from my experience having worked with more than 40 teams: "Code review is not the time for you to impose your personal taste on a colleague." There are always many acceptable ways how to solve a problem in software development, so if someone imposes his personal preference over that from a colleague this creates tension and a time sink for the team - don't make this a blocking review! This also plays a big role when developers obsess over code but lack the vision and experience to see that not everything will be a relevant factor for technical debt in the grand scheme for the business.

rileymat2 1y ago
There is a major exception to the personal tastes issue, consistency and predictably in a mature codebase. Things that would be personal taste in a greenfield project become important for maintenance in a mature product.
- watwut 1y ago
  Then make that explicit topic during the setup instead of taking random commits hostage to whatever catches your attention this week.
  - rileymat2 1y ago
    It is hard to fully specify all the different dimensions of consistency. Look at the code that exists and write in that voice. Maintenance will be much better.
    - watwut 1y ago
      It is even harder to chase personal consistency feelings of multiple different colleagues. Usually inconsistent with each other. Usually enforced only against less assertive committers and not enforced against those perceived to be strong. That is why you should either make it a rule or let it be.
    - hinkley 1y ago

moltar 1y ago

Is anyone else using conventional comments https://conventionalcomments.org/ https://conventionalcomments.org/

shagie 1y ago
I'm a proponent of them. They help separate out atomic commits ("I'm writing the feature here... I'm doing a refactor there"). They also serve for being able to quickly identify where I need to look in the story I told myself for "what did I do there?" While the goals that it is designed to get people to follow is something that I've preferred in my own commits in the past, this gives a structure to them that helps identify when there's too much in a commit for it to be reasonably reviewed in isolation.

deleted 1y ago

[deleted]

awithrow 1y ago

I think a lot of this stems from the code review tools themselves. Especially the "Don't just review the diff" mistake. Especially with GitHub defaulting to just showing a list of diffs and changed files. I'd find it much more useful if a review tool started with a class hierarchy or similar high level view to get a sense of: 1. What/how many classes changed? 2. What/how many methods have been added/removed/modified 3. What method signatures have changed 4. What changes are covered by new tests "Don't leave too many comments" i think can really be rethought of, don't review style and syntax. Leave that to the robots. If you're relying on other engineers to flag style problems and linting, you're just wasting everybody's time. Set up linting and style checkers and be done with it.

hinkley 1y ago
A martial arts instructor does not tell you everything you're doing wrong on the first day. You would quit. And justifiably so. What an asshole. You know you've progressed in their eyes every time they start bugging you about something new. You didn't suddenly become worse at something. Rather you got good enough on some higher priority thing that they knocked it off the list and replaced it with the next item in the backlog. You should treat code reviews similarly. It's a journey, and we are in the middle. If you keep making the same comments on reviews, eventually they'll get addressed beforehand and you can point out something else.

dchuk 1y ago

I feel like a useful tool someone should build now that LLMs are so capable, is some sort of automated walk through of a pull request, where it steps a reviewer through initially the overview and they why of the changes, then step by step through each change, with commentary along the way generated by the LLM (and hopefully reviewed by the pull requester for accuracy). Then the reviewer can leave comments along the way. I’ve always found the cognitive overhead of going through a pull request challenging, seems like the paradigm can shift now with all of the new tooling available to all of us.

clickety_clack 1y ago
It seems like maybe you should just talk to your colleague who wrote the code. It would be far more efficient than having them create and review a whole review experience on top of the work that they did, and then have you try to understand a walkthrough document (that might have hallucinated explanations that the requester didn’t quite catch) plus the underlying code, plus still have to talk to them to ask clarifying questions anyway.
- thunky 1y ago
  This sounds like you're making an argument against pull request descriptions in general. Like you just want to see code changes and have a meeting about it.
  - 1718627440 1y ago
    You can have both sync and aync communication. You can also talk to people without scheduling a meeting.
    - thunky 1y ago
      > You can have both sync and aync communication Sure, and PR descriptions and comments are a way to do that (async). But IME it helps to start with a good description of what the changes are rather than just having a pile of code changes dumped on you where you then have to reverse engineer the intent. It seems draining to me to have to have to start from nothing and interrogate someone for every PR that comes in. > You can also talk to people without scheduling a meeting I didn't say anything about scheduling.

OptionOfT 1y ago

I'm a little stricter. In terms of formatting etc I rely on tooling which break the CI. That way they don't even surface. Equally if something slips through the CI needs to be updated. A piece of code should match the general style of the code it is being integrated into. When you create an addition to a wood-framed house the addition won't be steel construction, unless you have an explicit reason (weight, speed, foundation etc). And if I'm the main maintainer of a codebase, it is something I can call out on. If we use Tokio and you use raw threads I'll ask about this choice. In an ideal world you (the PR author) includes this in its description, because it is a reflection of your understanding of the codebase. Reviewing code with 'will this work' is also debatable. Someone might propose a change that fixes an issue they are having. But maybe it's not something we want to solve in our codebase, or maybe it breaks other things the maintainers are thinking off.

maleldil 1y ago
> In terms of formatting etc I rely on tooling which break the CI. That way they don't even surface. Equally if something slips through the CI needs to be updated I also do something similar with lint rules. For Rust, this means turning on every single reasonable clippy lint (there are many), and making all warnings into errors. Some would say that some rules are overzealous or annoying, but it's a price I'm willing to pay to keep the codebase uniform.
- OptionOfT 1y ago
  Same here, I have a Cargo.toml in which I painstakingly went through every lint and decided whether it made sense to me.
hinkley 1y ago
I contributed to an OSS project recently that ran the linter before the tests and it absolutely trashed my code-build-test cycle. If I felt less strongly about the value of the project I would have bailed. If I’m trying to get the code to work don’t cockblock me on Make it Work with your Make it Right enforcements.

AndyNemmity 1y ago

The thing that always bothers me is when the comments involve things that have nothing to do with the PR. Like, they are reasonable ideas, but open up a new issue. If every reviewer wants to tackle large topics in the PR Review that have nothing to do with what specifically is happening, then it explodes and gets even harder for others to review now that we are changing things that have nothing to do with the change.

hinkley 1y ago
It can be tricky to make a change in a file that is considered to be rotten. Especially for high priority tickets. Yes, we want to clear all of this code up. But you have to be very careful not to punish people for trying. I had to tell one guy to knock it off because I was the only person brave enough to touch certain files and he was quickly making me regret trying by marking blocking comments on things I already planned to address in the subsequent or following PR. But I have to keep the old and new stuff working at the same time, boyo, so tap the brakes.

dakiol 1y ago

It's not that complicated: - you review and if to the best of your knowledge you think something can be done better you comment about it and leave a suggestion on how to do it better - then you approve the PR. Because your job is not to gatekeep the code

arccy 1y ago
words by someone who has never needed to be responsible for running code. no wonder people like ai vibecoding.
- jakewins 1y ago
  There is a middle ground here though. I try, anymore, to make sure I’m very clear on which review comments are blockers for me and which - most - are style, suggestions, questions. Unless the code has some egregious problem I immediately approve it and write a comment saying “this is good by me if you fix blockers A, B”; that way they are not then blocked by me doing a second review unless they want it. Maybe this is because I’m working somewhere where we don’t use stacked reviews though? So it’s a major pain for someone to have a PR open for a long time going through lots of cycles of review, because it’s tricky to build more work on top of the in-review work
  - arccy 1y ago
    i guess there's also a difference in the places you work at. the places i work at expect trunk to always be clean, and ready for production (continuous delivery). if you work someplace with a slower release cycle, then getting a not perfect change in may be more acceptable. there's also responsibility, which is traced during incidents back to author and reviewers. i won't approve until i'm confident in the code, and that will mean the author needs to answer questions etc.
- hinkley 1y ago
  Some people don't know what responsible means. To some people, "I take full responsibility," means, "I will publicly admit to being wrong," instead of, "I will do everything in my power to clean up this mess that is my creation."

Learner100 1y ago

> When you receive a review with a hundred comments, it’s very hard to engage with that review on anything other than a trivial level. Wow, that seems crazy. I can only hope I never have to work with somebody who thinks it is productive to leave that many comments on a change -- I genuinely cannot imagine any change that could ever require that. Great article, fully agree with all the points.

arccy 1y ago
usually it means your pr is too big and/or you didn't discuss it enough beforehand...
- Learner100 1y ago
  In those situations, the more productive option to course-correct is to talk to the change author in a meeting/chat instead of just volleying off a tsunami of comments about various minutiae in the change IMO.
5Qn8mNbc2FNCiVV 1y ago
I've had some PRs that would have required a ton of comments. There are usually two ways I handle this: If the PR went in a completely different direction and missed the goal by a lot, I take ownership of it (with a brief explanation) and re-implement it. I then use the new PR for a pairing session, where I walk through both PRs with the original author for learning purposes. If it’s mostly smaller issues, I schedule a half-hour pairing session with the author and review everything together, after preparing a list of issues. Doing it any other way puts too much burden on the author to guess what the reviewer wants, and it slows down velocity significantly.
hinkley 1y ago
When the PR system is being used as a weapon to freeze someone out, everything has gone to shit.

suralind 1y ago

I disagree with the point „don’t leave too many comments”. I think it’s easier to read a code that is a) following repo standards, b) following community standards. If a PR requires a 100 comments, so be it. Granted, I also almost always give a benefit of the doubt to the author unless a change is really required and I only ever seen a PR with close to a 100 comments once or twice. I should also say that you shouldn’t leave n comments with the same content so usually there’s no need for more than a couple comments in total.

cadamsdotcom 1y ago

There is opportunity in the meta. If you are leaving a comment it’s because someone didn’t learn how things should be from the tools in your codebase. Naming style problem? Linting isn’t catching it. Architecture violation? Your import linting (checkers to prevent route handlers importing the DB and skipping the service layer, etc.) isn’t catching it. Duplicate method? What tooling can you create in the LLM era - remembering you can ask agents to build scripts now?

criemen 1y ago

Great article, and I mostly agree. I want to nitpick on > Don’t review with a “how would I write it?” filter _Thinking_ about "how would I write it?" when reviewing code catches a lot of stuff for me - I want to be able to come up with how I'd do it, see how the PR does it, and then check if the approach matches. If not, there's something to be learned and explained here. Although, all of that is in my head! If I come to the conclusion that the presented approach is wrong or inferior in an objective way (say, doesn't handle corner cases neatly or at all), I'll comment. Otherwise, let people do their thing! So, I agree with the point of not using this to generate a lot of comments, but I do believe in it as a useful technique to engage the brain, instead of just loosely scrolling over the diff.

hinkley 1y ago
A navigable code base is one where people can either 1) guess where a particular bit of functionality should be implemented, 2) find a paper trail that leads them to the spot where it is. If you can't find it, you can't fix bugs. If you don't find it, then you end up splitting related functionality across several places in the code. Which will introduce bugs, and make them a pain in the ass to diagnose.

ramanvarma 1y ago

I'm not convinced code review should be about "consistency" or "system memory" at all. Half of the dysfunction in large codebases comes from reviewers over-optimizing for long-term purity instead of short-term progress.

mock-possum 1y ago

Code review is the job, in a lot of ways - whether it’s code you wrote, code a coworker wrote, or code generated by an LLM - you are asked “is it okay if this code joins the codebase?) It’s no small responsibility.

kiliancs 1y ago

I see where this sort of take comes from. Reviews are not a place for subjective comments that just slow down or block progress, signaling, excessive nitpicking, etc. However, the reviewer should be thinking beyond the immediate "hey, this is progress", and also consider how this change affects the codebase and how it aligns with feature and technical direction. A few happy approvals and a great codebase becomes hard to work with, if contributors are inexperienced. Code reviews are also an opportunity for learning, and provide real-life scenarios to inform decisions that are otherwise not grounded and mostly opinion. Of course, not all comments and discussions should block approval, but there is a balance. No one is going to die if this feature is delayed by a few days if you get a better result _and_ are investing in team growth.

3abiton 1y ago

> Many software engineers now spend as much (or more) time reviewing the output of their own AI tools than their colleagues’ code. This hits home.

deleted 1y ago

[deleted]

hinkley 1y ago

> If tons of PRs are being blocked, it’s usually a sign that there’s too much gatekeeping going on. I would expand this and say if too many of the blocking statuses are coming from one reviewer, too much gatekeeping may be going on, even if the overall rate is low. Either they're going too far, or nobody else is going far enough. If you're shouting into the void, discretion is the better part of valor. You can't save people who don't want to be saved. Find people who will appreciate your efforts instead of resenting them.

hinkley 1y ago

> For many engineers - including me - it feels good to leave a blocking review, for the same reasons that it feels good to gatekeep in general. Clearly this is not true for everyone, but I don't think I've ever felt good about gatekeeping, and a number of people I respect generally don't either. Blocking is a weapon of last resort. That someone has had to use it means either that the system isn't working, or they're abusing the tool. Someone shouldn't arrive at the final phase of their ticket and be told to start it over. That's a retrospective item, and if it's happened multiple times then this is an agenda item at the retro meeting.

rkomorn 1y ago
> Blocking is a weapon of last resort This is exactly how I feel. The only times I would block a PR is if I know it will either: a- actually break something existing b- not deliver what the PR says it should deliver (eg new feature but the feature doesn't actually work) It's basically saying "I'm certain that if we merge this, something bad will happen and we'll either cause an incident or need a hotfix".
- hinkley 1y ago
  c - it represents a complete failure to adhere to a standard of coding that we all approved as a group. Any of these things are a negative commentary on your process, or the members of your team. When one finds oneself standing in a hole, it is important to stop digging.
  - rkomorn 1y ago
    Agreed on c- . The rest seems a bit strident. Stuff happens. I wouldn't exactly go running to the incident/retro unless there was repeated pattern.

blks 1y ago

If I see that majority of code is generated, I reject the PR