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

PR process killing morale and productivity

blackentropy.com

Post reply on HN
Print view View original

111–120 of 224 posts

Re: PR process killing morale and productivity

#111

Post by lkrubner »

dmurray wrote: post #81
lkrubner wrote: post #58

When it comes to code reviews, the return on investment faces the Law Of Diminishing Returns. While many of the comments made in code reviews might be interesting, they are not so interesting that they pay for themselves. If you put some dollar value on the time invested, you'll find that the vast majority of this process is simply burning money. And not only money, but also, as this article says, morale. A curious f…

> I've come to a similar conclusion. Do not write a comment in a code review, instead, write a test that would catch whatever danger you want to warn about. And if you cannot find a way to express your concern as a test, the return-on-investment of worrying about that concern is probably zero, so we should ignore it. What if your concern is "this approach makes the code difficult to test"?

You discover that by writing the test. You don't try to predict that ahead of time. The goal is to get away from trying to predict problems ahead of time, because most of those problems turn out to be phantoms.

Re: PR process killing morale and productivity

#112

Post by sibit »

I have always found articles/discussions of code reviews fascinating. In the 10(ish) years I've been employed as a programmer I've never worked in a department with more than ten people and every time the work is distributed such that each person works as an IC on their own siloed project(s). Receiving anything besides the "LGTM" rubber stamp or nitpick comment seems almost impossible.

Re: PR process killing morale and productivity

#113

Post by theshrike79 »

OptionOfT wrote: post #80
barbazoo wrote: post #5

> I’ve recently come across a discussion where a new developer joined a team and faced over 300 PR comments on their first contribution. Most of it was stylistic nitpicking. This isn’t just unproductive, it’s outright toxic. For me this says more about the company culture than any inherent flaws with the code review process.

This is why I love things like cargo fmt / go fmt / eslint / etc. No discussions of whether if (foo) return; is valid, or we should do if (foo) { return; }

Just having a valid .editorconfig for the project will do a lot for languages without gofmt.

Re: PR process killing morale and productivity

#114

Post by wnoise »

The_Colonel wrote: post #99
bluefirebrand wrote: post #96

Earlier quoted context omitted.

> The solution is to just not be too anal about it. It really is a cultural problem "Any proposal that requires everyone to just is not a solution, because everyone will not just" People are anal. You aren't going to get them to stop being anal A real solution is to have the team agree on a shared style guide, then enforce it with a linter and formatter. If anyone cannot come to an agreement with the rest of the team…

This: > "Any proposal that requires everyone to just is not a solution, because everyone will not just" invalidates this: > A real solution is to have the team agree on a shared style guide, then enforce it with a linter and formatter. ... since the "team" is everyone. It's basically the same problem. The other issue is that linters/formatters don't "solve" all formatting/stylistic choices. Most formatters, fortunate…

You're not wrong.

But in theory, agreement and buy-in is a one-time thing, while actually writing the code and reviewing the PRs are constant things.

Re: PR process killing morale and productivity

#115

Post by bluefirebrand »

The_Colonel wrote: post #99
bluefirebrand wrote: post #96

Earlier quoted context omitted.

> The solution is to just not be too anal about it. It really is a cultural problem "Any proposal that requires everyone to just is not a solution, because everyone will not just" People are anal. You aren't going to get them to stop being anal A real solution is to have the team agree on a shared style guide, then enforce it with a linter and formatter. If anyone cannot come to an agreement with the rest of the team…

This: > "Any proposal that requires everyone to just is not a solution, because everyone will not just" invalidates this: > A real solution is to have the team agree on a shared style guide, then enforce it with a linter and formatter. ... since the "team" is everyone. It's basically the same problem. The other issue is that linters/formatters don't "solve" all formatting/stylistic choices. Most formatters, fortunate…

They aren't the same at all

A shared style guide is an external impetus to adjust behavior. It comes with external accountability, and also an implicit understanding that violating the shared expectation may bring consequences

"Everyone should just be less anal" is expecting an internal impetus to adjust behavior. There no external accountability, and there's no expectation that failing to do so has consequences

> The other issue is that linters/formatters don't "solve" all formatting/stylistic choices.

80% of a solution is better than 0%

Re: PR process killing morale and productivity

#116

Post by Brian_K_White »

Jare wrote: post #97
hibikir wrote: post #92

Earlier quoted context omitted.

That's also why I dislike it: The mind that will go apeshit over trivial style nonsense will still provide unhelpful feedback on other topics... but it will become far less obvious that their comments should be downgraded. Formatters also do a poor job if your language is flexible and expressive. Great for go, but if your language is the kind that easily supports internal DSLs, formatters are not even helpful at maki…

> it will become far less obvious that their comments should be downgraded. I'm curious why you think this would happen... I would imagine that their comments will now have be to about things that matter, and if they are unhelpful they will stand out more.

It is obvious that a style comment is of no significance, and that the person who made it chose to spend time and personal capital on something of no significance.

It is not obvious that an actual code change is of poor quality.

To show that takes infinitely more experience, work to analyse all the direct and indirect ramifications of both the original and proposed approaches, capacity to push back and decide that your own engineering judgement is equal or better than whoever is purporting to correct you, willingness to suffer everyone else accusing you of arrogance for that on top.

Even with all of that, it's a lot harder to prove the valueless comment is valueless because the more you know, the more you know that practically every single line of code could be done 30 other ways with some valid argument for each one.

It's completely insidious both for the junior and the senior.

The junior has no way to know the junk comment is junk. So they internalize the comment and everyone is worse for it.

The senior has it almost worse. They know enough to know they don't know everything and the comment might be valid, so they put in all kinds of work to try to figure out if they actually missed something, did they have point etc. Maybe in the end the code doesn't end up as bad as the junior just rolling over, but it sucked for everyone and the challenge was not really an intentional productive crucible, it was just a douche that everyone had to waste time and energy taking seriously.

Only someone who actually is arrogant (whatever level, whichever side of the pr) has it easy. Again bad for everyone except them maybe.

Re: PR process killing morale and productivity

#117

Post by bluefirebrand »

arp242 wrote: post #98
bluefirebrand wrote: post #96

Earlier quoted context omitted.

> The solution is to just not be too anal about it. It really is a cultural problem "Any proposal that requires everyone to just is not a solution, because everyone will not just" People are anal. You aren't going to get them to stop being anal A real solution is to have the team agree on a shared style guide, then enforce it with a linter and formatter. If anyone cannot come to an agreement with the rest of the team…

You can't "enforce" all of these things. That was my main point. And you absolutely can stop people from doing that. Simply accepting dickish asshole toxic behaviour as "well, people are like that shrug " and never telling people off is exactly the problem.

If you start telling people off at work, there is a good chance that you will be perceived as being the problem

If you go to management to complain about a coworkers behavior you may just be told that you have to adjust your expectations to get along with them

If there's a team agreement and someone continues to violate it then you actually have a complaint you can make to management that has some bite to it

Re: PR process killing morale and productivity

#118

Post by bluefirebrand »

chuckhend wrote: post #38
bluefirebrand wrote: post #13

Earlier quoted context omitted.

Honestly for me it tells me they need to hook up a code auto formatter into their workflow Forget stylistic nitpicking. Enforce a code quality standard with a linter and formatter and be done with it

Automating this with linter and formatter is great. It moves the argument over style and format to a one liner change to a lint config instead of mingling it with the with the main code change.

It also hopefully only happens once.

If you continue to have people bringing up arguments over the linter and formatter after an initial agreement is made, then you can talk to those people

Re: PR process killing morale and productivity

#119

Post by erik_seaberg »

yjftsjthsd-h wrote: post #48
dec0dedab0de wrote: post #21

Earlier quoted context omitted.

* I've seen good/great people call out the nitpicks (in my case it was often mis-spelling, due to not being a native speaker of english) but will approve the PR anyway (implicitly expecting another revision to be sent, trusting the submitter).* I always thought this was the best way. I wish these systems had a way to assign severity to comments, and urgency to the commit. If you have a jr developer it is your job to…

Depending on your company culture, you can also just explicitly write something like "This is not a blocker, but I would suggest...". Of course if your culture uses explicit "press the button to unblock" then that's probably redundant.

Half of my comments were "nit: consider blah blah" where I want it on his radar, but I approved anyway and if he declines I'm fine with it.

Re: PR process killing morale and productivity

#120

Post by bsder »

Kinrany wrote: post #33
jchw wrote: post #20

Personally I have a strong distaste for projects that try to use some metric for how long a function should be, e.g. line count or cyclomatic complexity. I'm not sure if this one is better automated, human judgement for what makes sense seems better to me. Sometimes the cleanest and highest performance way to write some code is going to be basically one relatively large function. If there's a function complexity/size…

It shouldn't be a hard rule, but in most cases a sufficiently long function will have opportunities for pulling out functions with sensible signatures that make sense outside that context. All a linter does automatically is require adding the "ignore this line" comments that are then visible to humans in diffs.

> in most cases a sufficiently long function will have opportunities for pulling out functions with sensible signatures that make sense outside that context.

And will make understanding the function a lot more difficult.

If you want an example of this, look at any C++ code that uses DX12 or Vulkan. It will be atomized sufficiently that you'll have to hunt through dozens of files to figure out which constructor/destructor where something fired. Part of the problem is that C++ discourages naked functions operating on structs rather than member functions on classes.

Whereas, if those functions were still all linear in a single function, you'd find what you were looking for in the same file you were already in.

That having been said, I'll probably organize that big function as though it were separate functions. But, then, I'm not allergic to adding an extra block level here or there which GASP might exceed 80 columns.

Post reply on HN