Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Looking at Our Nitpicks

datto.engineering

Post reply on HN
Print view View original

11–20 of 36 posts

Re: Looking at Our Nitpicks

#11

Post by Conan_Kudo »

sesuximo wrote: post #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.

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...

Re: Looking at Our Nitpicks

#12

Post by giraffe_lady »

binwiederhier wrote: post #5

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 t…

> 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.

Re: Looking at Our Nitpicks

#13

Post by nightpool » 

    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.

Re: Looking at Our Nitpicks

#14

Post by cube2222 »

Conan_Kudo wrote: post #11
sesuximo wrote: post #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.

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...

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.

Re: Looking at Our Nitpicks

#15

Post by LukeShu »

Conan_Kudo wrote: post #11
sesuximo wrote: post #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.

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.

Re: Looking at Our Nitpicks

#16

Post by wffurr »

nightpool wrote: post #13

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…

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.

Re: Looking at Our Nitpicks

#17

Post by withinboredom »

rileymat2 wrote: post #8
codeduck wrote: post #6

Earlier quoted context omitted.

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.

Re: Looking at Our Nitpicks

#18

Post by matt7340 »

sesuximo wrote: post #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.

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.

Re: Looking at Our Nitpicks

#19

Post by munchbunny »

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.

Re: Looking at Our Nitpicks

#20

Post by matt7340 »

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.

Post reply on HN