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

31–36 of 36 posts

Re: Looking at Our Nitpicks

#31

Post by sunny3 »

withinboredom wrote: post #4

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

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.

Re: Looking at Our Nitpicks

#32

Post by sesuximo »

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

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.

Re: Looking at Our Nitpicks

#33

Post by sesuximo »

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

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

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.

Re: Looking at Our Nitpicks

#34

Post by 3pt14159 »

onion2k wrote: post #7
3pt14159 wrote: post #2

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.

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…

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.

Re: Looking at Our Nitpicks

#35

Post by shkkmo »

nightpool wrote: post #29
ketzo wrote: post #28

Earlier quoted context omitted.

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

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…

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

Re: Looking at Our Nitpicks

#36

Post by onion2k »

3pt14159 wrote: post #34
onion2k wrote: post #7

Earlier quoted context omitted.

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…

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.

Post reply on HN