Stop Nitpicking in Code Reviews

(blog.danlew.net)

42 points | by ingve 5 days ago

15 comments

  • throw149102 5 days ago
    I'm not sure how much I agree. The author says there are 2 big benefits:

    1. Vastly improved signal-to-noise ratio 2. Better relationship with code reviews

    For 1, in most CR systems you can mark the comments as resolved once their fixed. They don't even have to show up once they're fixed. That should improve the signal-to-noise ratio. In addition, nobody should really just be glossing over any comment, regardless if it's a nit or if it's a serious change.

    For 2, I still think that you should try to not take code reviews personally. Perhaps it's more a sign of the environment or the company these CRs are happening in? That "zen-like acceptance" isn't something that just pops out of nowhere, it's something that has to be developed and built. Strangely, when I get a bunch of nits on my CR, it actually generally makes me happy. It's proof that the other developers on my team couldn't really find anything wrong with the code I wrote.

    Of course, I'd still love to have 0 comments instead of a bunch of nit comments, so I 1000% agree with using automated tools to lint code, ideally in multiple places (before build, before commit, after commit, before merge, after merge, ...). I think it also helps focus reviewers on giving more meaningful feedback.

    • testesttest 5 days ago
      Write software for another 10 years and I bet your opinion will switch. Nits don't produce substantial value to the business or the code base. Add some decent automated tooling and be done with it.

      That being said, I would have agreed with you years ago.

      • tharkun__ 5 days ago
        I would like to provide 'the middle ground' so to speak :)

        I think it depends on the 'nits' and the environment. "pointing out a variable name that could use a more appropriate word" as the article puts it, is not a nit if you ask me. It's something that is just part of Clean Code. Now the way that you message that you found such a suboptimal variable name plays a big role I believe as does the overall culture in the company and in the PRs. It will also depend on your relationship history with the guy pointing it out and whether you've worked with him in person/close contact or not. Friends can 'play rough' easily (what to an outsider might look like you're fighting) while actually having fun with each other. The exact same behaviour/communication would seriously turn off someone you don't really know very well.

        At my place for example, we collectively have a high standard on these things and it's absolutely not considered nitpicking to point out these things. You're generally grateful for them as it happens to all of us that we just use 'some name for now' while trying out an idea and we overlook one while 'cleaning up'.

        In the same vein, the "boy scout rule of coding" - "Always leave the code base in a better state than what you found it in" - can either be applied in a good way or can be taken way too far. It's all about the balance. Just like in the real camp ground, sure you will pick up the trash you found in the fire pit and when you find some more trash while wandering about camp, you'll pick that up too and throw it out. But you're not gonna laboriously interrogate every square inch of the camp ground plus a two mile radius around it, are you? You'll never get anything done that way.

        • bluefirebrand 5 days ago
          If a team has a set of coding standards and for some reason does not have automated linting tools to enforce them, then nitpicks in code review are the only way to really enforce the standards.

          Overall, this makes the codebase much easier for the whole team to work on. That's why we make code standards in the first place.

          • dozzman 5 days ago
            I also would have agreed with OP 5 years ago. Working in a leadership capacity for a prolonged period has emphasised the human side of coding, that is working in a team and respecting your colleagues’ freedom to think independently. IMO it’s far more important to maintain a strong and happy-to-work team than a marginally more aligned-with-my-own-thinking codebase.
          • anothernewdude 5 days ago
            I have two major rules that address these:

            The first is that code review comments are leaping off points for discussion, and filling in the space next to the comment with a justification is a valid fix. (And "I don't understand this" is a valid comment) This helps equalise the two roles, and makes it just as valid for a junior to review a senior's code, and avoids artificially limiting who can review any piece of code.

            The second is that everybody reviews everybody else's code regularly. The source of people taking them personally (in my experience) are people who haven't had their code reviewed in a while or having the same people do all the reviewing.

          • BugsJustFindMe 5 days ago
            With the right definition of nit, I agree. However, the first example is "a variable name that could use a more appropriate word" AKA "a variable name that increased confusion". If perfect performance optimization doesn't matter, then don't review for it, but code readability does always matter. It won't be them who has to fix it in a few years, so take every opportunity to change things that put friction on the path of enlightenment to within the bounds of your capacity to sacrifice optimizations.

            It doesn't make you an irascible ass to say "I find this variable name confusing. Can we change it to something more like X?"

            • woutr_be 5 days ago
              > "I find this variable name confusing. Can we change it to something more like X?"

              My problem with stating it like this is that when the person saying this is in a position of authority it’s extremely hard to say no to it. And that person might not always be necessary right, or the person writing the code had a good reason for this naming, and now can’t argue against it.

              While there are best practices for naming, people often have different opinions, but that doesn’t mean one is right and the other one wrong.

              • BugsJustFindMe 5 days ago
                > it’s extremely hard to say no to it.

                Good. A person just told you that you did something that confused them. You don't have to rename it to "X" exactly, but you do need to try to do something that communicates effectively to your collaborators.

                > And that person might not always be necessary right

                There is no right. There are only confusing and not-confusing. A person that you're collaborating with just told you that what you did is unclear to them. That means you have to make things more clear. I don't care if your solution to this problem is to just add a comment, as long as you're both happy that the end result communicates your ideas effectively. But you have to do something to fix the fact that your original variable name didn't communicate what you thought it would.

                > that doesn’t mean one is right and the other one wrong

                Thinking of names as objectively right or wrong is a mistake. Names either add confusion or reduce confusion. Striving for the latter requires meeting your interlocutor halfway.

                • woutr_be 5 days ago
                  > A person just told you that you did something confusing.

                  The person told me that I did something confusing to them. And with using that kind of format, almost forces you to have to pick his opinion. Because now instead of clarifying your choice of naming, have to now defend your choice against his or her.

                  > There is no right/wrong here. There is only confusing and not-confusing. A person that you're collaborating with just told you that what you did is _unclear_ to them. That means you have to clarify what you did.

                  A person just told me they find a name confusing, and asked if it can be changed. That person could’ve clarified why it was named a certain way, but instead just proposed a change.

                  Again, the way the comment was written makes it extremely hard to do anything other than use the proposed new name.

                  > Thinking of names as right or wrong is a mistake. Names either add confusion or reduce confusion. Always strive for the latter.

                  That’s an extremely subjective statement, and to me is not how things are in the real world.

                  • BugsJustFindMe 5 days ago
                    > And with using that kind of format, almost forces you to have to pick his opinion.

                    I said that it needs to communicate the right information to both of you, not just to you and not just to them. If their suggestion still communicates to you, then suck it up and admit that they came up with a better name. If it _doesn't_ but you use their suggestion anyway, then you've just failed at being part of a team. It's on you to learn how to say "that's unclear to me, let's look for a third option".

                    > have to now defend your choice

                    It sounds like you think you're likely to feel defensive about having created confusion for someone. Maybe focus less on defending and more on achieving clarity.

                    • woutr_be 5 days ago
                      > I said that it needs to communicate the right information to both of you, not just to you and not just to them.

                      Right, but you see how it took multiple back and forwards to even clarify what you wanted to achieve?

                      The same goes with your original comment.

                      > "I find this variable name confusing. Can we change it to something more like X?"

                      You don't clarify why it's confusing, so I don't know your interpretation of what the variable represents. I need to first understand as to why you find this confusing, and how your new proposed naming will help to clarify.

                      > It sounds like you think you're likely to feel defensive about having created confusion for someone. Maybe focus less on defending and more on achieving clarity.

                      I do not, and it's pretty bad practice to assume this of people. This comes back to the whole nitpicking, to me it sounds like you were nitpicking on the naming. You didn't clarify your stance, or what caused you confusion. And leave it up to me to reach out to you to clarify this. This could've been communicated in your first PR comment, so that we didn't have to go through this dance.

                      • BugsJustFindMe 5 days ago
                        > Right, but you see how it took multiple back and forwards to even clarify what you wanted to achieve?

                        Luckily we're in this together having a conversation with an interest in achieving mutual clarity.

                        > I need to first

                        That's ok! You should feel comfortable expressing your needs. That's the nature of discourse. I guess if I were to turn your argument back around on you I would say "you could have expressed your need from the start."

                        But nobody but me is inside my head and nobody but you is inside your head, and neither of us knows what the other needs until we tell the other. So we do our best to get there eventually. I tell you that your chosen name doesn't make sense to me and propose an alternative that does. Maybe you say "ok, that works for me too", or maybe you say "that doesn't work for me but if you tell me more about the problem maybe there's another alternative." That's how teamwork goes.

                        But if my alternative suggestion _does_ also communicate to the other person but they demand that I convince them anyway because they want to be "right", well, then that person is just being hostile and defensive, which makes being on a team more difficult.

                        • woutr_be 5 days ago
                          > I tell you I need a different name. You tell me that knowing why will help you come up with solutions. That's how teamwork goes.

                          I totally agree with that. And if this was happening quite often, I would somehow propose a better process for these kind of naming changes. Communication is always key, especially when it comes to written text.

                          > I guess if I were to turn your argument back around on you I would say "you could have expressed your need from the start."

                          That's kind of my point, if you had clarified your stance in your original comment, then we wouldn't have to go through this and instead could've focused on coming up with an appropriate name. In that sense I can agree with the OP on why this could be considered nitpicking.

            • nickm12 5 days ago
              By definition "nitpicking" is not desirable. The problem is what is the magnitude of improvement that bridges the gap between a nit and a substantive improvement?

              When I started off as a code reviewer, I would frequently comment when "I would have done it differently". Now I make sure I only comment when "I would have done it better" and I always explain why it is better. Writing that explanation forces me to ask myself, "is this better?" and I also really want it to be substantially better.

              There are a category of changes I request which I think of as "nits". Usually these are cosmetic changes that I think genuinely improve the code quality. I think they are worth asking why should we have a misspelling in a documentation string when we could not have it? It's just going to annoy everyone who reads that code until some unrelated change decides to fix it. When I have these kinds of comments, I preface them with "nit:".

              • Bananaman28 5 days ago
                I do this at work, even with newer developers, although I try give them more slack and time to adapt to our commit & PR style. Do I need to pick on minor formatting issues or a spelling mistake? Do I want to? No. My concern is that the long term cost of maintaining and fixing this when it was actually cought in PR.

                So whenever someone nitpicks on my code it's usually for the better, i.e changing a method name.

                • sunir 5 days ago
                  Just fix the spelling. Only comment what is worth the time talking about.
                • mgerullis 5 days ago
                  Thanks for this post. It’s very relatable.

                  I started working around 5 years ago in a startup. I was a bloody beginner and we had someone joining who was pretty senior, who I suspect had OCD to some degree.

                  Being a startup we needed to ship, quickly.

                  That senior developer joining was working quite differently. He did not believe in auto-formatting, but he cared a lot about things which did not degrade the end product by the least bit.

                  E.g.: He had this super specific way of ordering CSS properties. It kinda made sense but I had to look it up every time and we did not have a linting rule for it.

                  He kept blocking PRs due to this, or a tiny typo in some comment. Being British he also kept correcting grammar.

                  He regularly found some extra breaking space or non-breaking space which was off. It meant that the PR is not gonna go through, until resolved.

                  Often, my PRs comment count would shoot past 50, at times even hundred, and I was becoming more and more desperate.

                  Today I tend to say: if it’s not lintable, it’s not a codestyle rule, and it’s been pretty great ever since.

                  • LilBytes 5 days ago
                    'He kept blocking PRs due to this, or a tiny typo in some comment. Being British he also kept correcting grammar.'

                    Being British. I feel personally attacked.

                    I assure you, his nationality had no correlation to his banal ways. Unless he was trying to correct your spelling of organization to organisatio which I can forgive.

                  • anarcat 5 days ago
                    I recently realized that I was spending more time telling people what I wanted them to do in their merge requests than I would spend just merging their thing and patching it the way I want.

                    So, for small projects I manage on my own, this totally makes sense: instead of saying "this comment has a typo" or "could you rephrase this message", I find that it's easier to just rephrase the thing myself than do another round-trip with the contributor.

                    It makes everyone happier: I get things exactly the way I want them, and the contributor doesn't feel like they're wasting time on trivialities, while still getting their contribution acknowledged.

                    The "suggest change" feature in GitHub and GitLab does do some of that, by the way, so for me that's a viable alternative.

                    And of course this works only if you're the sole (or one of few) maintainers who can commit directly to the HEAD branch. If you need to do another merge request to fix those issues, that process doesn't work so well, obviously, and there are some errors that just need to be fixed.

                    But I found that it pays to be less "nitpicky". It certainly makes for a more relaxed ambiance. :)

                    • daredoes 5 days ago
                      I like to preface my nits with `nit: `, while trying to have an understanding that nitpicks can be ignored
                      • null_deref 4 days ago
                        Same, and I immediately approve the PR, if only my nits are left, while trying to have an understanding that no major changes are going to happen.
                      • jimmyvalmer 5 days ago
                        Previous discussion: https://news.ycombinator.com/item?id=22074357

                        It's neither possible nor desirable to reprogram nitpickers, many of whose obsessive compulsive tendencies are precisely why they're good programmers. That's why I institute the policy that reviewers must counter-submit a PR based on the original PR's commits. You want the change, you make it yourself.

                        • iamevn 5 days ago
                          The code review software I use at work lets reviewers add suggested edits that can be applied with a single click by the author of the commit. I try to use this as often as I can and mark any nits as non-blocking so they can still submit without another round trip if they decide against fixing for whatever reason.

                          I think nits are valuable but are absolutely not the focus of code review. Personally I'd rather see a few small nits on review of my code than just approval because that suggests the reviewer engaged with the code for more than just a quick glance.

                          • Jakobeha 5 days ago
                            This is the way. There's nothing wrong with code reviewers correcting minor details, the issue is when said corrections waste the author's attention and time and slow PRs.

                            If the edit is an easy bug-fix, the author should just look over it for a sec and approve. If it's a style change in some cases you don't even need the author to look at it.

                            • ris 5 days ago
                              The "single click applied suggestion" thing of course doesn't work if you have a policy of avoiding trivial commits (actually a very good policy) and your PR is already about 3 or 4 commits. Then the author has to sit down, figure out which commit a change belongs to & do a `rebase -i` to squash together associated commits...
                              • iamevn 4 days ago
                                Why wouldn't the edit just get appended to the end on the chain in the PR?
                                • ris 4 days ago
                                  It would, but that's not what you'd want in this case.
                            • ris 5 days ago
                              The worst kind of nitpicks are ones that address lines that weren't even touched by your PR.

                              (And that's not to say it's not sometimes useful to ask someone to fix something else they noticed, but to nitpick on it is the worst.)

                              Getting good at code review is about learning that just because something isn't exactly how you'd do it, byte-for-byte, doesn't mean it's not "fine".

                              • elanning 5 days ago
                                Couldn't agree more. If you care enough to nit, you should care enough to add an linter rule for it. I do realize sometimes that's simply not possible, but a lot of times, it is.

                                I even wrote a custom vscode plugin to encourage devs to do just that: https://marketplace.visualstudio.com/items?itemName=checkr.c...

                                Now it only takes me 10-30 minutes to whip up a new lint check.

                                • pseudoramble 5 days ago
                                  As time has gone on I’ve become a bit better about being too particular. I still sometimes point out nit picks, but critically I ask myself if I could live with what was there. If I could, then I put my approval on it and move on. If it’s something more critical, then I wait for a response, fix, or discussion about it. Seems to make the reviews easier and more worthwhile.
                                  • nicbou 4 days ago
                                    I am very nitpicky, but I also understand the frustration of dealing with nitpicking.

                                    Having a common linter saves a lot of trouble.

                                    For everything else, I explicitly "prefer but don't insist". It's up to my colleagues to decide if the feedback is important.

                                    • amw-zero 5 days ago
                                      Here’s the thing. If someone spent a week writing code, and the first time you’re seeing it is at code review, the chances of nitpicking as well as dissenting on the actual design go way up.

                                      Talking over the plan in advance is much more efficient than after the fact code review.

                                      • caseyross 5 days ago
                                        I also think it's important to think about the other side of this, which is why nitpicking happens in the first place. In my experience, the underlying reasons why people nitpick code have little to do with shipping the best possible version of the product --- rather, nitpicking is a response to:

                                        1) pressure from PMs/POs for quick, yet "effective" code reviews that don't hold up releases, and

                                        2) implicit pressure from other developers to maintain "polite" relations by only pointing out minor, "typo"-level issues with their code instead of requesting architectural changes that could be seen as condemning their work.

                                        • sgtnoodle 5 days ago
                                          #2 there sounds rather terrifying. I'm not sure I understand #1. Is the idea that folk will nitpick in order to appear like their code review was thorough for their managers?

                                          I've mainly worked on safety-critical systems, and thorough code review is generally expected and welcomed in that space. In my experience, perceived nit-picking is rooted in two things. 1. Lack of consistent style guidelines or unwillingness to conform to style guidelines. 2. Arbitrary design decisions that weren't vetted by anyone until code review.

                                          The former can be generally managed by agreeing on expectations outside of individual code reviews, and then referencing the relevant documentation when pointing out style problems. The latter can be managed by encouraging folk to communicate their plan and solicit feedback long before they submit fully baked code for review. Not to say that folk shouldn't just knock out code if they're motivated to, but without earlier communication they must be willing to defend their code in review.

                                          As a reviewer, it also helps to annotate comments. If you feel like a comment might be a nit or might be perceived as a nit, just call it what it is i.e. "nit: this variable name could be better". At the end of the review, if it's all just nits, give your coworker an "out" by saying, "looks great! There's just a bunch of nitpicks, so feel free to address how you see fit and ship it." Or if it's more than a nit but not a bug, "Overall this looks pretty good. There's one thing there with a potential divide by zero if the config file is bad, but if you're in a hurry I'd be happy with a TODO comment".

                                          To reiterate, managing expectations is important for avoiding code review drama. Typically with new hires, their first few PRs I review often end up with upwards of 50+ comments, most of them style related. The first thing I do before even looking at the code, though, is send them a message saying that I am a thorough code reviewer, I'm not trying to give them a hard time, and their first few PRs are likely going to be a bloodbath since they're new to the code base. Afterward, most folk usually go out of their way to thank me for being upfront about it, and that they really appreciated the thorough feedback.

                                        • mjbeswick 5 days ago
                                          If a comment is a nitpicking then it should simply be a optional suggested change.