TFA:

> Engineers [..] request reviews when they think it's necessary.

Problemo solved.

It does however mean you don't sit around waiting for someone to approve a three line delete PR that you know is safe. The "Rubber Stamp" review is a thing and it does waste time.

I'd be exactly in the boat you describe (and actually: I've yet to see a situation where someone reviewing my code did not lead to improvements). But: If I want my code reviewed, I have to actually fight for it. Since everybody's workload is too high, even the willing often simply don't have the time.

From higher up, at our place, there's no one that actively opposes code review as a practice, but they don't seem to get that it requires investment (i.e. time), either. That means that if management refuses to change their ways of planning or explicitly enshrine reviews as priority tasks, it simply won't happen except for maybe the gnarliest cases.

And that's from a perspective of the change's author being willing and proactive. I work with individuals that produce very questionable code at times. And I'm not talking style here: Code in 2021 that still is prone to SQL injection at every turn. Methods that are hundreds of lines long with a gigantic cyclomatic complexity that no one (including the author) will ever really understand. Method names like "process" and "process2". And so on.

Needless to say, those programmers will never push for their changes to be reviewed. The rest of the team simply discovers them after they blow up (which they do regularly) and we're debugging.

Now, these people are luckily a tiny minority (which at least makes this somewhat bearable), but I over and over again would wish for someone with more authority to show that they're interested in the matter. Which a policy on code review would do -- if only as a signal. Yes, we want to check each other's code. Yes, we want collaboration. Yes, that's more important than some arbitrary deadline for minor feature X. Yes, we expect our programmers to be professionals.

Another symptom of such an environment also is the fact that everyone is the king of their particular hill. No one every familiarizes themselves with another person's code unless they really have to. By now, I've jumped to insisting that every change on a critical component I'll ever make will be done through pair programming, and so far, at least that seemed to stick. But again -- that only happens if both parties involved actively fight for it instead of everybody understanding that this is just an expected part of work.

Was interesting to hear your experiences and maybe update my expectations of workplaces somewhat

> [...] alongside the culture of my employer [...]

I think you've hit the nail on the head here. As many others have said in different HN posts, culture is hard to change. And I would also understand the argument that one can also enforce culture changes with policies; my hope up to now simply has always been that engineers could "overrule" some artificial time limits in favor of more collaboration by referencing such a policy.

I guess if you're starting from a context where code reviewing practices "just work", introducing explicit policies also can't improve much, anyway. On that note: If you happen to know a good heuristic to detect such cultures during interviews or the like, I would be extremely interested :)

This hits at the heart of why I strongly agree with this post and think PRs and code reviews are ultimately destructive.

Reviews are going to get dropped _regardless_ of whether they are in the process or not. Pressure makes code reviews and quality worse, not better.

More than that, the need to "appear" to review is going to slow things down and can cause an exponential back up. I saw this happen about a mouth ago: 3 day release cycle got bogged down and then took _3 weeks_ to get a single release out.

Process goes out the window when pressure hits. You are then operating in foreign environment at the most critical point and your safety net of useless.

Your safety net when things need to go fast should be highly automated fast build and deploy, coupled with great test coverage, from unit to end to end.

The only exception might be if you work in highly regulated field where you legal required to have a very low Mean Time To Failure. In that case things move slow unless you are very well resources.

Otherwise (ie the majority of products) focus that effort on having a very low Mean Time to Resolve. If you absolutely need some form of quality assurance, Pair. Do continuous PRs and knowledge sharing rather than out of context after the fact reviews.

There is an easy way to resolve this:

     Request peer review for SMALLER chunks of code.

Each small change on its own might make sense and be ok to a reviewer, but if you step back and look at the series of changes together there might be some issues that could have been noticed.

Then that's what you need to fix.

...when they need it. Actually, really good engineers will ask to pair when they need it. Review is a distant second.

I agree with you there, but I'm also sure that one reason for this is that many people still see reviews as the task that only happens after everything else.

I'm convinced there's a lot of useful middle ground to be had if reviews aren't just "here's 500 LOC, please review", but maybe more… staged.

Say: Rummage around in the code a bit for yourself (ask questions if you have to), come up with a strategy of solving your problem, and submit that for review. Not in code, but maybe using markdown, preudocode, small diagrams or the like. In person (i.e. via screenshare nowadays). And if the other party doesn't find obvious holes in your strategy, then go implementing it.

I remember an interview with some Linux kernel maintainer that mentioned the same effect -- people submitting a gigantic patch for something that will simply never be merged. And all that would have been necessary to avoid this wasted work was an e-mail asking if the maintainers are at all interested in the change.

One downside of code review is reduced velocity. There are teams out there who use code review so effectively that they never ship big fuck-ups to production. They never have to tell clients that they have bad practices. Instead, they struggle to get clients because they pay much more per line of code shipped and therefore don't deliver as good of a value as their competitors.

This where the argument needs to be: whether and how to have code reviews be a net positive.

I can see how that equation balances in favour of no code review when it's in the context of a startup that is probably still figuring out how to make money, especially with a subscription based app launcher. As far as prototyping and early stage iterations go, you're probably just seeing what works and reviewing the code isn't so important at that point. There would still be one or two things you'd ask someone to check, e.g. security related stuff.

I think you make a good point though: what makes an effective code review process? I don't think there's a single answer to that because it depends on the circumstances.

I would at least say that it goes better if it's treated as a priority and issues are raised and dealt with promptly, rather than leaving it as a chore you eventually get around to. And you would have to see value in it beyond it being a sanity check, as others in this thread have described (knowledge transfer, for example).

We really should be relying on automated systems to catch bugs for the most part, and leave code review for what it's good for: making sure a different human can understand and maintain the code.

In fact, if we manage to catch a bug in a code review that didn't also trigger our automated systems to alert us, that should probably prompt us to look into how we can expand the coverage of that automated system, similar to what we'd do in a postmortem if the bug wasn't caught in code review and reached production, because we can't rely on that happening the next time.

Disagree, but it's also a good place to be ensuring that the system is evolving in a way that is reasonable and consistent — or that it is unreasonable and inconsistent for good reasons.

I’ve always code reviewed/gotten code reviewed on both the implementation and the tests, with completeness as the main quality you’re reviewing test code for. If that’s how we do it, we’re right back at code reviews.

I'm not saying code review can't find any bugs (in practice bugs are found in code review all the time), I'm just saying that if we do find a bug through code review, it means we got lucky, and we shouldn't count on getting lucky again, so better add a test/linter rule for the next time when we might not get so lucky. That's just one more way to grow your automated systems organically.

What big launch? :) Is useful to feature flag new functionality so that you can release gradually and spot the bugs - often via error monitoring and user feedback - before they affect everyone. Yes, realize it can happen but there are techniques to minimize the chance of it happening.

However, the review process is far from perfect and can create a false sense of security.

To review a piece of code, that code should ideally be small in scope. For larger pieces it's rather common that the reviewer don't have enough time to do a good job.

If there were better incentives for doing a review, then it would greatly improve the situation.

But i haven't seen that yet.

In almost all cases it's very hard to get people to look outside the web browser for the diffs. Diffs show you something, but never the whole context.

The other problem I've found is stamping down on style arguments: they have no place in code reviews, and a lot of the time you get a round of "please change this variable to be named something else" - which might be valid, but raises the question of why the reviewer didn't do it, and more often leaves a weird authority gap - I have no power to just reject such a request because I think it's invalid, nor does the business have anyway to resolve the issue. In fact if there's a dispute at all, very rarely is there any process for bringing in a third party to mediate or break the stalemate (which is hilarious in some ways since everyone by now should realize 2-party systems can't achieve consensus).

Code reviews are very, very cargo-culty and I've not seen them ever include a thorough design: people give up on the very near edge cases, and so the whole system falls apart because it's just a hurdle to get through and not a collaborative or constructive process.

Or are you saying that one can actually find more bugs when two people compile and execute the same code rather than just one?

When I review code, I want the author to tell me how they have verified that the code has the desired effect -- but if they do that, I'm going to trust that they did the actual verification and I'm not going to simply retrace their steps. That seems like a waste of time to me.

1) Catch edge use case / business requirements misunderstandings

2) Mentor new hires/junior engineers on the ways things should be done (e.g. internal culture about some architecture/design).

BUT, I absolutely despise code reviews that nitpick on non documented or 'because I prefer that way' stuff:

1) Task tickets that only have a vague description, have their own 'play detective' about the requirements, but a lot of new ones surface during code review.

2) Requirements are met, but there are internal/personal technical directives that are written nowhere, are not enforced with automated checks and may dramatically change a feature/patch is implemented (I can only think about that guy that worked in a company in which only LEFT JOIN sql statements were allowed).

Fuck that noise.

The main purpose of code reviews is checking the general structure of the code and tests. Is the code well designed? Are error cases considered and tested? Another important purpose of code reviews is knowledge sharing.

To spot quality problems before going to production .. what you need is testing.

Not "unit testing" - which is just another pointless process that does not actually provide the purported benefits.

You need a separate QA team that handles testing - manual and automated.

You should absolutely have a small QA team, but their job shouldn't be doing the QA -- it should be helping others do their own QA.

One thing I've seen that I like a lot is individual changes creating individual front ends. You test the one thing in that change, and it's done. This tends to cause a lot of problems with CORS, but then what doesn't?

Agreed with you though that good code reviews (whatever that means) obviates most of this.

Your quality problems might well come from poor or non-existent architectural decisions and tech-debt, and QA won't pick up on that stuff unless it manifests as a faulty requirement.

The earlier you catch a problem, the easier it is to fix.

In our case we have a public API and several backends. The tests run against the public API. To know that a new backend is working it has to pass all the tests. Getting any new backend working correctly without the tests would be nearly impossible.

I’ve heard from my workplace’s software team that this makes hiring from certain sectors particularly difficult because they don’t get this. Too used to move fast and break things and MVP.