Looking at Our Nitpicks
datto.engineering Engineers work to deliver business value and commit the code that flawlessly delivers that value the very first time.
When colleagues go to review the code, they are edified in its reading and quickly approve the merge request.
There is no need for feedback because the code was perfect.
I'll often do the "first draft" as a design doc with an API sketch or a quick prototype that I'll post in chat or email to the team but not actually "mail for review" per the tool. That way I get early feedback that I'm going in the right direction for a code change.
When it comes time to actually get approval to check in, then I try very hard to get it right. I put on my code reviewer's hat and review on my own code. Ensure that I've run the formatter, linter, pre-submit tests, and code coverage tools. And only then do I actually mail it for review.
My ideal review at that step is a rubberstamp. I have a few colleagues who do the same; it's always a joy to review their changes because they're focused and well-written with tests, coverage, etc. all in place.
That makes sense for some types of changes, but it's too high-level to address the actual well-factored nature of the code, which is one of the main things I'm thinking about in the "second draft" of my code. Nobody's design doc is going to address "what's the right set of helper functions to extract here?" or "what's the right way to structure this logic to make it the most readable?". Design docs are great and they're super important for bigger changes, but they're often completely orthogonal to "is the actual code that implements this design doc readable and maintainable", which is the biggest question I'm seeking to answer in code reviews.I'll often do the "first draft" as a design doc with an API sketch or a quick prototype that I'll post in chat or email to the team but not actually "mail for review" per the tool. That way I get early feedback that I'm going in the right direction for a code change.This is also completely orthogonal to "do I have tests for this", "have I run the linter", etc. Certainly there are types of readability problems that running the linter or writing tests for your code can catch. Having focused changes with test coverage is only one part of making sure your code is long-term maintainable by the rest of the team.
> I'll often do the "first draft" as a design doc with an API sketch or a quick prototype that I'll post in chat or email to the team but not actually "mail for review" per the tool.
Yeah, we do a database & api design review before anything is actually coded. If you don't get that right you might have to basically start all over.
On the flip side, though, I think it's important for the code that you send to other engineers for review to be at least kind of polished, because otherwise you risk just wasting other people's time.
If you get feedback on your code that you already knew about, you wasted someone's time, and I think that's inefficient and a little bit disrespectful.
But I definitely agree that code review is very much a drafting process. It's just that you need to walk a line.
I get where you're coming from, but if a friend asks me to look over an important email that they're planning on sending to their boss, I don't think of it as "wasting my time" if the email has a bunch of grammatical errors in it. Instead, I'm happy that I'm able to help them by applying a fresh pair of eyes to the problem, since they've probably spent a really long time working on the actual content of the email and haven't had the opportunity to step back and consider . It's just an efficient division of labor. In the same way, code review is a great opportunity to let someone who wasn't involved in the actual writing of the code approach the problem & solution space with fresh eyes, now that you've done the hard work of translating the solution into something that actually works.
And code reviews are even more collaborative then that. When I'm looking over my friend's email, it's still ultimately their email and there are types of feedback that it would be inappropriate to give. But when you're contributing to a shared codebase, it's more like a novel that's been co-written by multiple different authors—obviously you want everybody on the team to be able to have input and feedback into what the "collective style" of the project is going to be.
> they've probably spent a really long time working on the actual content of the email and haven't had the opportunity to step back and consider
Before you ask someone to proof read anything, you should generally take the effort to step back and try to proof read it yourself. The same goes with code reviews.
I think there is absolutely value in soliciting feedback throughout the process, but would consider those earlier feedback stagea to be design and/or implementation reviews (which depending on feature size should happen with varying degrees of formality.) It is useful to be explicit about the types of review because the types feedback that are useful vary throughout the process. If you are looking for design feedback and someone complains about unimplemented tests, that generally isn't very helpful. If you are looking for a final code review for polished code and you get feedback about the underlying design, then you really needed to get that feedback earlier in the process.
> 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.
Imo, the code review is not the place to over communicate your preferences. Do it on developers meeting. Do it in a chat. Do it on standup. Don't do it in random code reviews where people unlucky enough to get you as reviewer have to deal with different expectations then anybody else and spend hours discussing your personal preferences - without any real impact on general codebase because other reviewers and coders have different preferences.
Maybe preference was the wrong word for me to use. “Reasoning” might make more sense?
If comments don’t impact the code base or the people involved, then yeah they’re not useful.
I'm conflicted on this. On one hand, I think it makes sense to over-communicate preferences because building consensus on these things is hard without it. On the other hand, I don't like how it causes everything to drag out...
Maybe it's worth trading off on going one way or another? Or maybe there's a happy medium somewhere...
The happy medium I've found is to clarify my level of preference; whether it's a "nitpick" or an actual "change request". I communicate that you don't have to update the PR just for nitpicks, but that if you're updating it anyway that they might be good things to also change.
A nice practice is to finish all such nitpicky comments with the strength of your opinion / how much you care.
This way, if most of those are "very little" then it's up to the author to decide whether to do it or not, and they don't have to wait for you to circle back about it.
I agree with the spirit of this, but the async nature of code review seems to prevent it.
Social and power dynamics in async code review can make things very difficult.
This is a big factor. I’ve experimented with in person code reviews but it’s kind of annoying to organize. Definitely open to ideas on this front.
I 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.
I remember I read in HN somewhere that, people _want_ to feel useful in code reviews and one way is to leave comments. So one strategy is to send stuff out early with some small imperfections so that people who want to feel useful can pick on them. IMO, following this spirit, and in this case, an efficient but not necessarily personally beneficial strategy would be send out a PR early (so that you're not investing in too much time and not too emotionally invested in defending your solution later), then get them to suggest solutions, and implement in their way. This takes away pretty much all of the pleasure of creation in coding and deprives one of the sense of accomplishment, but it's far less frustrating and mentally straining than having to argue and defend which solution to adopt, to me, when working with that kind of people.
Were they any better at finding edge cases or was their non-nitpicking feedback particularly insightful?
The teams i'm on have a rule about nits, "only fix nits if you are going to touch the code again". So if there's a misspelled word in a comment or a formatting/code conciseness cleanup, it can be ignored unless there are other actual bugs to fix.
It works out pretty well, the marginal cost of fixing the nits is very low when there's other changes to be made, so they get done. If they aren't addressed because there are no actual problems with the code, those things will get addressed later. (all the members of the team tend to take cleanup passes occasionally when they aren't feeling up to the deep work stuff but still want to do something productivish). The net effect is the codebase stays reasonably tidy without feeling bogged down in the "bureaucracy" of nitpicking.
I think I’ve reached the point where I simply don’t mention most nits. I’m most business software it just doesn’t seem worth it. If the code works, isn’t utterly obtuse, and abides by automated formatting etc, then is the nit really worth it?
Granted I’m probably biased towards high churn SaaS apps, which is what I work on.
An old DBA once said to me, "You only have so many keystrokes before you die. Don't waste them."
Somethings are not worth the taps.
This 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.
The ideal code review process goes something like this:
- Engineers work to deliver business value and commit the code that flawlessly delivers that value the very first time.
- When colleagues go to review the code, they are edified in its reading and quickly approve the merge request.
- There is no need for feedback because the code was perfect.
- The business then benefits from the code expeditiously.
I think this is a misunderstanding of the many purposes reviews serve. Your goal isn't to produce a pull request that attracts no comments and requires no revision, even though that's what it might feel like because revisions take extra work. Other than the pedagogical goal, your goal is to take advantage of the team to build something better and more efficiently than you could by yourself.
Reviews that take multiple revisions are only bad if a lot of time is being spent on careless mistakes. But driving (forcing) meaningful discussion about the structure of the code and whether there are any edge cases the code missed is the point of the review, and an ideal one helps the team tackle them head on before the code goes into production.
Very 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.
> IMHO, code should be written so that there is no visible personal style
This seems reasonable at first but depending what you mean by visible personal style can get you into the weeds so fast. You can end up creating a process where no minor, specific improvements or experiments are tolerated unless they can be justified as generalizable improvements for the whole org.
> , so that newcomers and new engineers are able to follow what's going on regardless of who wrote it
This isn't actually related to the first thing? There's some overlap sure but for the most part whether it's in camel case or snake doesn't affect my ability to comprehend it.
> This isn't actually related to the first thing? There's some overlap sure but for the most part whether it's in camel case or snake doesn't affect my ability to comprehend it.
This isn't about camel case or snake case. It's about the overall structure and look and feel of the code. When you know that public functions are at the top, constants are in a certain place, and where package-level comments are; and when you know that important abstractions and models are defined in one file, and their implementations in separate files; and so on.
Basically, it's the combination of all the tiny things that make code look and feel like they belong.
This is similar to when people say "this is not idiomatic Go". What I'm saying is pretty much "idiomatic for this project/company/product/team". Write it so that it matches the rest of the project. That helps people navigate and read your code, which is the most important thing.
The 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)
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.
Everything else being equal, I prefer shorter code. I agree that this example is both shorter and more readable, but it doesn't really matter in the grand scheme of things.
Everything else being equal, I prefer shorter code.
I appreciate that some developers do, but things are never equal. Your own preference should largely be irrelevant if you work on a team. Code is how developers communicate with each another, so readability and clarity for someone else who's reading the code should always be the driving force behind your coding style.
Often shorter code is actually easier to read, so it kind of works out, but it should never be the goal. I actively push back on code where clarity has been sacrificed for brevity.
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.
If you're using a language where there is a difference between value-equals and instance-equals, it could cause a very subtle bug by changing this code if the result of it was relying on an instance-equals later on.
I agree it can introduce bugs, my point is relying on that is probably a design issue somewhere else.
Because even in the first, you are relying on the order of max being the same everywhere it matters.
Maybe. If it’s usage were in the scope of the current PR, I might suggest a fix like this. But if it might introduce something subtle, I probably wouldn’t suggest it or I’d ask if they had considered changing it as well as if there were any subtle issues to worry about.
As someone who works in a multimillion line codebase that sees millions of executions daily since 2005, these are real issues and saying “it’s a design issue somewhere else” doesn’t exactly fly because that “somewhere else” may no longer have a team maintaining it.