4 ms·

As someone who worked on code bases with sometimes fairly big teams before the days of mandatory code reviews: I applaud this. I feel like the culture of code r

• zelos 5y ago
  Most of the Nit level issues should probably be caught by the linter/static checker, which reduces a lot of the minor comments. Not that that seems to stop some people, of course.
• nine_zeros 5y ago
  Nitpicks by devs with incomplete information are the worst. They have little context but end up blocking a PR for weeks because they think it should be solved differently or are not convinced. After 5 weeks of debates and discussions, the original PR is merged with little to no modifications. But everyone lost time.
• quartz 5y ago
  Not sure of your personal experience with this but mine was that the era before code reviews was mostly enabled by having often large teams of QA / SDETs who were burdened with finding the output of bad code and throwing bugs back at the eng team (see: the famous account of winword engineers pushing known bugs into the code to increase perceived velocity with the expectation to get a QA report back to fix them). That era also required more documentation effort since knowledge wasn't shared at the time the code was written and best practices weren't as well enforced around making sure code is readable since there was never a "test" of someone else looking at it. Kind of feels like these folks are trying to distribute that QA role across the company (everyone dogfoods the latest versions). I imagine this would work well so long as the team is reasonably small and the product is simple enough that everyone uses every feature of the app often enough to find small bugs and corner cases, but I still wonder if at some point enough issues will arise in prod that could have easily been caught earlier such that someone will eventually say "maybe we should just look at each other's code and catch this stuff before it's committed?" Would love to see a followup from these folks in another year or two.
  • davedx 5y ago
    Yes and no. If you have mostly senior people who take responsibility for the code they write, then the amount of stuff getting picked up by QA will be more or less the same IME. Code reviews catch some stuff but they can also slow down your team’s velocity disproportionately to the bug/arch issues it catches. YMMV of course. Some orgs have much more sane and pragmatic code review practices than others.
• JoshuaDavid 5y ago
  At my workplace all code review comments must be classified as either "blocking", which is stuff like "this code will break the app for our users" or "this is insecure", or "non-blocking", which is stuff like "this method is hard to read and should be broken up" or "I think this variable could be named better". Non-blocking stuff is (really, truly) optional to address. IMO this system works pretty well as a balance between "we want to make sure nothing catastrophically bad hits production" and "we don't need perfect code, just good-enough code". It does require that you have a general understanding on the team that the purpose of the code is to deliver value to your users, and does not have to be perfect to be valuable. We did also at one point try a system with more granular classification where every comment had to be prefixed with one of a predefined list of things like "nit", "security", "performance", "comment", or "kudos", which was fine but I don't think really adds all that much relative to just "blocking / non-blocking".
  • bcrosby95 5y ago
    > "I think this variable could be named better" I had a friend describe to me how hurt he was because the code reviewer didn't like how he named things. Apparently he built a giant metaphor for his system. And he then wrote code in that metaphor. I don't recall the details. It involved fruit, trains, clippers, etc. He was writing a system to report on how disk space was used across their systems.
    • aliasEli 5y ago
      Names are important for understanding code. They are most useful if everybody uses the same conventions. It sounds like your friend was trying to push personal conventions onto others.
    • dwaltrip 5y ago
      Did he eventually realize that his naming system was confusing and could be cleaned up?
  • stephane-klein 5y ago
    https://conventionalcomments.org/ https://conventionalcomments.org/
• schwartzworld 5y ago
  There are two kinds of NITs. The first kind should really just be linter rules. If it's not important enough to enforce across the project, it isn't worth mentioning now. The second are strongly held opinions the other person doesn't want to make a big deal about. Maybe they are expecting pushback, or not feeling confident in their explanation of the issue. So they couch it in a nit. You should be able to push back on comments you disagree with. Often I'll bring up suggestions that I think would improve the code but that I don't think are must haves. I think we can share a tremendous amount of knowledge in code review. And of course, the easiest way to get fewer code review comments is to thoroughly review your own code with a critical eye. Review it like its someone else's. Find the typos and unclear variables. Anticipate the "nits" that you know are coming.
• greatgib 5y ago
  I completely agree with you. There was a time were a software developer was trusted as a responsible adult company employee. I think that what changed is that, before, devs were mostly experienced trained professionals, and young ones were expected to be seriously trained before being "trusted". Now, dev work has been commoditized with the influx of "web dev shops" and easy things like js and nodejs allowed a lot of mediocre script kiddies to be considered the same as professional developers.