Exactly. I want my code peer reviewed partly because it makes it clear and formal that while we succeed as a team we also fail as a team. It’s much easier to talk about failures when a failure doesn’t have a single person attached to it.
TFA:
> Engineers [..] request reviews when they think it's necessary.
Problemo solved.
...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.
Then that's what you need to fix.
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 :)
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.
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.