[HN Gopher] Looking at Our Nitpicks
___________________________________________________________________
Looking at Our Nitpicks
Author : mcrowson
Score : 39 points
Date : 2022-02-14 13:50 UTC (9 hours ago)
(HTM) web link (datto.engineering)
(TXT) w3m dump (datto.engineering)
| nightpool wrote:
| 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 strongly disagree with the premise of this article. I look at
| code review the exact same way as I would look at the first draft
| of an email or a blog post. If you look up any piece of writing
| advice, one of the biggest things people say is "Don't worry
| about getting everything perfect the first time". Instead, it's
| really important to focus on getting _something_ working first,
| and then you can spend your "second draft" refactoring that base
| of working code so it's more maintainable & readable. From that
| perspective, find code reviews really valuable as a contributor
| because it lets me get an outside perspective on the "first
| draft" of my code, and then it'll make it way way easier to write
| the code "correctly" the second time. So I really really want to
| push back on this idea that the "ideal" code review process
| involves a rubber stamp. That's just entirely foreign to the way
| I think about code reviews.
| wffurr wrote:
| 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.
| bcrosby95 wrote:
| > 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.
| nightpool wrote:
| 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.
|
| 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.
|
| 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.
| ketzo wrote:
| 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.
| nightpool wrote:
| 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.
| hallway_monitor wrote:
| 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.
| sophacles wrote:
| 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.
| 3pt14159 wrote:
| 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
|
| 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.
| codeduck wrote:
| The first returns bar if foo == bar, the second is dependent on
| the implementation of max(). There's a potential difference in
| behaviour.
| rileymat2 wrote:
| 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.
| withinboredom wrote:
| 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.
| rileymat2 wrote:
| 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.
| withinboredom wrote:
| 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.
| onion2k wrote:
| 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.
| 3pt14159 wrote:
| 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.
| munchbunny wrote:
| _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.
| binwiederhier wrote:
| 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.
| giraffe_lady wrote:
| > 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.
| binwiederhier wrote:
| > 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.
| matt7340 wrote:
| 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.
| hobs wrote:
| 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.
| sesuximo wrote:
| > 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.
| matt7340 wrote:
| 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.
| sesuximo wrote:
| 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.
| Conan_Kudo wrote:
| 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...
| cube2222 wrote:
| 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.
| LukeShu wrote:
| 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.
| watwut wrote:
| 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.
| sesuximo wrote:
| 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.
| withinboredom wrote:
| 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.
| pc86 wrote:
| Were they any better at finding edge cases or was their non-
| nitpicking feedback particularly insightful?
| sunny3 wrote:
| 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.
___________________________________________________________________
(page generated 2022-02-14 23:02 UTC)