If you got _reamed_ you got off lucky: in plenty of places you'd be out on the street.
It may well be that you had it right but from context as given I hope this shows you some alternative perspective that might give you pause the next time you decide to throw out the rulebook, even in emergencies - especially in emergencies - these rules are there to keep you, your team and the company safe. In regulated industries you can multiply all of that by a factor of five or so.
But not being in a regulated industry doesn't mean there isn't a very good reason to have a code review in your process, assuming it is used effectively and not for nitpicking.
Not having a code review step is usually a bad idea, unless everybody on your team is of absolutely amazing quality and they never make silly mistakes. I've yet to come across a team like that, but maybe you are the exception to the rule.
Why? Because it's a "good practice"? They have 12 people and no customers, they can almost certainly adopt a very aggressive developer cycle that optimizes almost exclusively for happy-path velocity. You'd never do that at 50+ engineers with customers but for 12 engineers who have no customers? It's fine, in fact it's ideal.
> with massive downtime as a result.
They have no customers, downtime literally does not exist for them. You are following a dogmatic practice that is optimizing for a situation that literally does not exist within their company.
If you're wondering why software is crap: it is because every new generation of coders insists on making all the same mistakes all over again. Learn from the past, understand that 'good practice' has been established over many years of very expensive mistakes. 12 engineers is already a nice little recipe for pulling in 12 directions at once and even if they're all perfect they can still learn from looking at each others code and it will ensure that there are no critical dependencies on single individuals (which can bite you hard if one of them decides to leave, not unheard of in a startup) and that if need be labor can be re-divided without too much hassle.
Dogmatic adherence to a process that limits developer velocity and optimizes for correct code is very likely the wrong call when you have no customers.
I don't know the context and I don't know the particular business the OP is talking about. What I do know is that if you feel that your management is cargo culting development methodology (which really does happen) you can either engage them constructively or you can leave for a better company. Going in with a confrontational mindset isn't going to be a good experience for anybody involved. Case in point: the OP is still upset enough that he feels it necessary to vent about this in an online forum.
Note that this is the same person who in another comment wrote:
"On the flip side I’m trying to convince my CTO to fire half our engineering team - a group of jokers he hired during the run-up who are now wildly overpaid and massively under-delivering. With all the tech talent out there I’m convinced we’d replace them all within a week."
> Case in point: the OP is still upset enough that he feels it necessary to vent about this in an online forum.
I apologize for the conversation starter.
Your attitude towards code-review on the other hand is one I've seen before several times, and I was glad when each of those people were fired.
If we waited for reviews before merging, we’d be waiting weeks to merge a single PR. Thus, you wrote your code, opened a PR, did a self-review, then deployed it. We had millions of customers, downtime was a real possibility. So you’d watch metrics and revert if anything looked slightly off.
You would wake up to your PR being reviewed. Sometimes there would be mistakes pointed out, suggestions to improve it, etc. Sometimes it was just a thumbs up emoji.
The point is, there are many ways to skin this cat and to “ream” someone for merging without deploying is incredibly immature and uncreative. You can still review a merged PR.
> to “ream” someone for merging without deploying is incredibly immature and uncreative.
I'd agree, but I _highly_ doubt that description was an accurate one. Read through the other comments by the same person and you'll get a picture of their personality pretty quickly.
It's likely that there was already an ongoing conflict either in general or specifically between them about this issue. They probably got a moderately harsh comment to the effect of "hey, you're expected to wait for code-reviews now, knock it off"
You want to get the team to a point where you can disagree and commit, no code will ever be perfect and there is no reason spending 3-4 rounds of change requests trying. I think the worst code review I ever had, ended with me saying, “if you’re going to be this nitpicky, why don’t you take the ticket?” (It was extremely complex and hard to read — and there wasn’t any getting around it, lots of math, bit shifting, and other shenanigans. The reviewer kept making suggestions that would result in bugs, and then make more suggestions…)
He came back the next day and approved my PR once he understood the problem I was trying to solve.
Even these days, where I work on a close team IRL, I’ve been known to say, “if there are no objections, I’m merging this unreviewed code.” And then I usually get a thumbs up from the team, or they say something like “oh, I wanted to take a look at that. Give me a few mins I got sidetracked!” And I’ve even heard, “I already reviewed it, I just forgot to push approve!”
Communication is key in a team. Often, if the team is taking a long time to review, give them the benefit of the doubt, but don’t let yourself get blocked by a review.
Anything else is un-needed grooming that's more about the other developer's ego, not about good code (sometimes its to follow some other constraint, but its a good sign the person has a personality issue).
> They have 12 people and no customers, they can almost certainly adopt a very aggressive developer cycle that optimizes almost exclusively for happy-path velocity. You'd never do that at 50+ engineers with customers but for 12 engineers who have no customers? It's fine, in fact it's ideal.
12 engineers churning out code with no code-review at all? That'll produce velocity, for sure. It'll also produce not just an unmaintainable mess, but an interesting experiment, in which you get to find out which of your engineers are socially capable enough to initiate technical communication independently and construct technical rapport _without_ that process helping them to do so. Hope none of them hold strong technical opinions that clash!
During all this time, I've seen plenty of "small changes" having completely unexpected consequences, and sometimes all it would take to avoid would someone else seeing it from another perspective.
I’m not convinced bad code would get merged more often if we didn’t require approvals. I am convinced we’d deliver code faster though, and that’s what I’m trying to optimize for. Your company and engineering problems are not the same as mine.
I agree with everything you say here, but honestly it’s quite ineffective. I wish we had found something better than unit tests (often misleading and fragile) and the ability of a qualified reviewer to maintain attention and context that a real review entails.
The primary value I've seen across teams has been more on having shared team context across a codebase, if something goes bump in the night you've got a sense on how that part of the codebase works. It's also a great opportunity for other engineers to ask "why" and explain parts that aren't obvious or other context that's relevant. We'll find the occasional architectural mismatch(although we like to catch those much earlier in the design process) and certainly prevented bugs from shipping but if that's the primary focus I think a team is missing a lot of the value from regular code reviews.
At the low end of engineering, yeah, code reviews matter a ton and do catch bugs even if they’re basically just peephole inspections.