Looking at Our Nitpicks
datto.engineering
Looking at Our Nitpicks
1–10 of 36 posts
Re: Looking at Our Nitpicks
#2The types of nits I raise are the ones where I think the author of the pull request would appreciate as something they should do going forward. For example:
if foo > bar:
return foo
else:
return bar
return max(foo, bar)
Re: Looking at Our Nitpicks
#3> Strive to be intentional, focused, and sparing in our reviews.
I get this but I prefer the opposite. By over communicating your preferences, asking questions, leaving “if I follow this correctly” comments, and generally not holding back, you create lots of chances for discussion and knowledge sharing. And you’ll catch more bugs.
This is especially true when a junior dev is involved.
Re: Looking at Our Nitpicks
#4I worked with a guy who would decline any PR that wasn’t written the way he would have written it (Voice of the Engineer). Any PR he reviewed nearly tripled the amount of time it would take to get for the reviewee to merge something (I researched this to give them feedback about nitpicking). The worst was they didn’t even realize they were nitpicking until literally the entire team was telling them to stop. They ended up leaving the company, thankfully.
Re: Looking at Our Nitpicks
#5Very well written, and funny too.
I wish there was a way to automate some of the lower buckets too. Things like "project consistency" (see his "12 buckets"), for instance, are incredibly important to me personally. IMHO, code should be written so that there is no visible personal style, so that newcomers and new engineers are able to follow what's going on regardless of who wrote it. A human can easily determine if the style of code is the same, or if it's different. It'd be fantastic if a machine could do the same.
Re: Looking at Our Nitpicks
#6The types of nits I raise are the ones where I think the author of the pull request would appreciate as something they should do going forward. For example: if foo > bar: return foo else: return bar Should just be: return max(foo, bar) That's a nit because what they wrote was fine, but it's longer than it should be.
The first returns bar if foo == bar, the second is dependent on the implementation of max(). There's a potential difference in behaviour.
Re: Looking at Our Nitpicks
#7The types of nits I raise are the ones where I think the author of the pull request would appreciate as something they should do going forward. For example: if foo > bar: return foo else: return bar Should just be: return max(foo, bar) That's a nit because what they wrote was fine, but it's longer than it should be.
What you suggest is actually more readable than the if block, especially if the variable names are meaningful, which is why it's better. It's more obvious what's expected to return. There's no need to claim shortness as a virtue of good code; generally speaking it's not better to write less code. Unless you work for an organisation that code golfs everything there should be no expectation that code should be as short as possible.
Re: Looking at Our Nitpicks
#8The types of nits I raise are the ones where I think the author of the pull request would appreciate as something they should do going forward. For example: if foo > bar: return foo else: return bar Should just be: return max(foo, bar) That's a nit because what they wrote was fine, but it's longer than it should be.
The first returns bar if foo == bar, the second is dependent on the implementation of max(). There's a potential difference in behaviour.
I saw that too, but if there is a "real" difference in returning equal things, then it could indicate a way bigger problem in the design.
Re: Looking at Our Nitpicks
#9I worked with a guy who would decline any PR that wasn’t written the way he would have written it (Voice of the Engineer). Any PR he reviewed nearly tripled the amount of time it would take to get for the reviewee to merge something (I researched this to give them feedback about nitpicking). The worst was they didn’t even realize they were nitpicking until literally the entire team was telling them to stop. They ende…
Were they any better at finding edge cases or was their non-nitpicking feedback particularly insightful?
Re: Looking at Our Nitpicks
#10This is a great short little article on how code reviews can turn ugly. On my current team we have formatting rules checked on build. This sometimes annoys me but we never ever have white space or formatting issues to fix in pull requests.