Hacker News Reader

Comment by ssrc | Hacker News Reader

Devil's advocate: PRs are unnecessary and insufficient (this is the article's introduction).

For each claimed benefit there's another practice that is better suited, usually automatic, and should have been already implemented. For example, for catching regressions, a reasonably complete test suite. For formatting issues, IDEs and autoformat on commit. For latent errors, static analyzers, linters, run-time instrumentation, etc.

Things that cannot be automated, like understanding of the problem, design tradeoffs, etc. should have been discussed with the team before the PR, with or without pair programming.

Now, going back to reality, if all of the above is working, the PR should be a breeze and there wouldn't be much debate about them.

View on HN

I'm curious now: have you ever been in an environment where automated checks have completely or nearly completely replaced code review? Mind sharing a more comprehensive list of what's hiding behind that "etc." in your comment?

One reason for my curiosity is that the author isn't claiming humans can do any of those things you talk about automated tools doing better than the automated tooling does. They're talking about it as a learning opportunity to see how other parts of the codebase work, and as an exercise in communication and teaching, all of which I can see the value in. In other words, your claims and the concerns backing them seem to be orthogonal to what the author has actually written.

What do you think about that?

I have no strong feelings about PRs (usually). I was acting as devil's advocate to try to answer supremekurt question in a positive way (others have pointed out the negative ways) and I ended up writing almost exactly what the article says in "Code reviews. Isn't it the past?".

But yes, I've been in environments where automated checks and communication during the previous steps nearly completely replaced the need for PRs. The "learning opportunities" happened, but almost always outside a formal code review. 3 devs + PO, competent, limited scope, same room, whiteboards.

Nothing magical behind that "etc.". Formatter, linter, unit tests, integration tests and code coverage in CI to keep you honest. Load tests launched manually to detect performance regressions, memory leaks and race conditions. Specific tools vary by language and product. You can do much more but for most products it's probably over-killing.

> Things that cannot be automated, like understanding of the problem, design tradeoffs, etc. should have been discussed with the team before the PR, with or without pair programming.

This is the problematic step because management wants high "velocity" and "impact", I'd say it is not possible for all team members to have understanding of the problem, much less design tradeoffs.

And then there is churn. You have to explain every time there is a new team member that no, the fact that we use var in our Google analytics mess is not a good reason for you to avoid let and const in your code.

This is increasingly closer to reality. Some shops though... Whew code review is more of a weapon then anything else.

Good code reviews are essential for finding bugs, especially those that are hard to spot.

They also ensure that the conceptual integrity of the codebase remains intact.

Moreover, they allow a developer who is less familiar with the codebase to propose a solution, even if they might not fully grasp the entire context of the project or codebase. This is a significant advantage.

However, unfortunately, code reviews can devolve into hazing or become a means to establish power dynamics that shouldn't exist.

We should never assume the reviewer knows everything. A productive code review is more of a collaboration rather than a teacher-student relationship.

How can we automate issues like using a task UUID as a device UUID within the code? Or addressing an n + 1 problem when another API with batch functionality is available?

We can't automate those(yet)

I agree with you. I would argue that most if not all team dysfunction arises during reviews. Ive worked on places where senior engineers will allow bugs like you've mentioned through to production so they can catch the "hard off by one problem" to get the + and stick the - on someone else. Even serious bugs. Yep leads protecting themselves from ambitious seniors and seniors protecting themselves from juniors who shouldn't be juniors. Vindictive practices, gas lighting non-technical management, etc.

So with any decent system, the weakest link is the people using it. If you have adversarial people on your team, and in my experience, you probably do, sometimes the system surrounding code review is far more important. IE reviewers share the blame(better yet there is no blame shit just gets fixed), and hard rules around being an asshole need to be enforced.

I realize you are describing a healthy work environment. Unfortunately, I've yet to find one. I almost made one once, but a wave of clandestine hires destroyed it and it was time to move on. Pissed off the wrong narcissist...

Reply on news.ycombinator.com