Live data from Hacker News

PR process killing morale and productivity

blackentropy.com

51–60 of 224 posts

Re: PR process killing morale and productivity

#51

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.

Yeah I do similar and encourage colleagues/orgs to do the same.

Re: PR process killing morale and productivity

#53
post #12

Whenever I encounter a pull request that I find many issues with, I ask to meet with the engineer and review it one on one. More than half of the workplace problems engineers have is due to their introverted nature and their refusal to get on a teams/zoom call to explain their issues and get resolution. Comments are a really poor mechanism for teaching programming best practices.

At my last job, the rule was if the PR is big enough, get on a call and go over it in person. ideally with more than one "reviewer", to break stalemates. Worked really well!

I used to be that introverted guy, but a few years of pair programming completely cured that, and I'm now very comfortable discussing design in detail and at length, in a (if I may say so myself) friendly and constructive way.

If you've never discussed design with other people, it's a genuinely difficult thing to do!

Re: PR process killing morale and productivity

#55
post #33
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.

A linter directive also tells humans that somebody has decided that the function being long is acceptable in this specific location. It's been acknowledged. In many cases, that's all I want.

Re: PR process killing morale and productivity

#57
post #56

Combine this with a large timezone difference and you get a (literally) slow motion disaster. Imagine waking up and finding your change thoroughly reviewed but not approved because of a triviality. Not a great day.

My bias is towards merging. Trivial issues can be fixed in a future commit or PR.

Re: PR process killing morale and productivity

#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 fact about the politics of modern tech teams is that some of the same people who consider themselves "anti bureaucracy" are strongly in favor of this type of bureaucracy, even when it brings no measurable benefits.

My opinion of code reviews has become more negative over time, mostly due to the fact that I have not seen it achieve reliable positive outcomes.

I'll give specific examples:

https://www.futurestay.com had the worst code that I'd ever seen. When I joined the company I was horrified at the level of tech debt. And yet, for years, they had a rule that at least two engineers had to review every PR. So this terrible code, the worst I'd seen in my 24 years of coding, had been approved by two engineers.

And likewise:

https://openroadmedia.com also had very bad code, and also had a rule that nothing could be pushed to production until at least 2 engineers had reviewed the PR.

In both companies the code review slowed down the deployments, slowed the down the team, and created a culture of internal sniping, while leading to no improvements.

But how can I say the code was objectively bad? Because many bugs were found in production. And what would reduced the number of bugs in production? More tests, especially end-to-end tests. And so I eventually came to this conclusion: it is best to skip code review, and instead have the team invest that same time in writing more tests, especially high level tests. For the most part, if the engineers on a team are bad then the code reviews will also be bad, but if the engineers on the team are good, the the code reviews will not be needed. And in both cases, the path to improvement comes from writing the kinds of tests that ensure no bugs get into production.

As I've grown in my career, and taken on higher level management jobs, I've also realized that code reviews do not scale to large-scale leadership, but tests do. Code review seemed like a good idea when I was leading a team of 3 engineers, but not when I was leading a team of 30. When I was leading a team of 30, the only tool-of-oversight that worked for me was automated testing. And so I've concluded this is where the effort should be made. Sitting at my own computer, I cannot review the work of 30 engineers, but I can still run the tests on the various software, to see if they are passing. In particular, I can look at API interfaces and then change the dummy test data to break the tests, and this lets me see quickly thorough the test coverage is, and how prepared we are for unexpected shocks.

When I am running a team I will assign software developers the task of writing various tests, including high-level end-to-end stress tests. These tests sometimes reveal problems. We then respond to the problems. But we don't waste time responding to problems that have not been proven by a test, which is to say we do not do code reviews. Code reviews are all about predicting what might be a problem. But much of the concerns are phantoms. It is better to respond to real problems that have been revealed by tests.

I've also come to realize that software developers, quite naturally, develop strong opinions about questions of style, and yet these questions have no long-term impact on the health of the tech in the company. Other than enforcing the rules of some automated linter, the time spent on issues of style are 100% wasted. If you allow the team to spend even one minute discussing issues of style, then you are setting money on fire. But I know this opinion is unpopular, because software developers enjoy enforcing their own opinions about style. But it is all bikeshedding, it has no real-world impact.

I have the impression that there is some middle era, in the career of a software developer, where concerns about issues of style and organization tend to peak. The junior developer does not know enough to care about such issues, but somewhere between 4 years of experience and 10 years of experience, these issues feel important. I think you need more than 10 years of experience to see the waste. In particular, you need to run one large project where the team invests a lot of time into code reviews, and you need to notice how much tech debt builds up, despite the code reviews, to realize that code reviews do not offer a path forward.

On recent projects I've told the team there will be no code reviews, but instead we will focus on building tests. I get a surprising amount of pushback on this. Less experienced software developers get angry with me and tell me that I'm being unprofessional. In some sense they are correct, in the sense that "professional" refers to "standard norms that are accepted by a profession" -- I am deviating from those, clearly.

The most reasonable criticism I get is that code reviews could catch not just problems of style but problems of algorithms. What if, they ask, some junior developer introduces code that is ignorant of the implications of Big O Notation? What if they introduce code that runs in polynomial time? But I would ask, how do we know if it is running slowly? We can only know that through tests. So lets build tests that measure time, and stress test with large loads. Big O Notation offers an excellent example of where software developers can worry about the wrong thing: what if a junior level software developer writes code that runs in polynomial time but on data that will only ever have a few hundred records? While the algorithm might be sloppy, the code will run quickly because there are few records? In that case, any time invested to find a different algorithm will be a poor investment. Once I'm running a team of 30, the only thing I care about is whether code is actually slow, and I discover that through end-to-end stress tests, not code reviews.

Kent Beck, when he invented Xtreme Programming, also popularized the phrase "Do not write a comment, instead, write a method with an easy-to-understand name that communicates what the comment was going to communicate."

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.

Re: PR process killing morale and productivity

#59
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…

Linux C style guide has this summarized pretty well:

"The maximum length of a function is inversely proportional to the complexity and indentation level of that function. So, if you have a conceptually simple function that is just one long (but simple) case-statement, where you have to do lots of small things for a lot of different cases, it’s OK to have a longer function.

However, if you have a complex function, and you suspect that a less-than-gifted first-year high-school student might not even understand what the function is all about, you should adhere to the maximum limits all the more closely."

"Less-than-gifted first-year student" is btw a great bar for the conceptual complexity. Another one I've seen elsewhere is "An SRE not familiar with the code woken up at 3 AM to troubleshoot an outage".

Re: PR process killing morale and productivity

#60
post #52

I recommend using something like https://github.com/erikthedeveloper/code-review-emoji-guide in code reviews to signify what your expectation is exactly.

Can you share your experience with this kind of review process?

I can mirror what their docs say

> Using CREG (Code Review Emoji Guide) puts more ownership on the reviewer to give the reviewee added context and clarity to follow up on code review. For example, knowing whether something really requires action (), highlighting nit-picky comments (), flagging out of scope items for follow-up () and clarifying items that don’t necessarily require action but are worth saying ( , , )

Post reply on HN