[beefheart]

Unpopular (?) Opinion:

In most "modern" environments, with constant online updates, where shipped defects are not that costly anymore, code review is a net loss. You can just skip it. This study fails to compare with "no code review whatsoever" and it measures by "defects found" instead of "total effort spent" (i.e. the only metric that truly matters).

In code review, most developers will not do the kind of thorough analysis that uncovers serious bugs, if they actually did, code review would be too costly. Instead, disagreements on superficialities and bike-shed arguments are promoted, often leading to low-value follow-up changes that can easily cause worse issues than they solve.

Code review satisfies the urge for process and structure, but it also satisfies the need for diffusion of responsibility, which is a bad thing. People need to own their code.

Code reviews have some value during onboarding or with junior devs, to get everybody on the same page. After that, you can disregard it, the effort is better spent on other things like automated testing.

[falsedan]

In my experience, code reviews

1. Check that the code is addressing the problem at hand and is fulfilling a business need (mostly making sure it’s linked to a reasonably-written ticket)

2. Align style and make code look consistent across repos/teams (which includes pointing out similar work which could be reused)

3. Force other people to read and understand the change, so the original author isn’t on the hook for all support/oncall questions)

4. Highlight hard-to-understand sections that might be unclear to someone newly-arriving to the code, suggesting those parts need more explanation or a simpler implementation

5. Copy editing on error messages and docs to remove typos/passive-aggressiveness/make important information easier to see

6. Find bugs by humans running static analysis (“this will race/deadlock”) and provide pointers to resources to help the reviewee learn (“see <some link> for why and how to avoid it”)

7. Human-run linting (if the automatic linters miss/mess up stuff)

I’ve also see it used as a way to bully other less-knowledgeable devs by nitpicking and issue-raising when the commenter isn’t included on the original list of reviewers and leaves criticism without any constructive feedback (“this is wrong”)

Rarely have we spotted bugs in code review: that’s what tests, oncall, and a robust rollback mechanism are for

[dasmoth]

mostly making sure it’s linked to a reasonably-written ticket

This one seems to come up quite often recently, but to me sounds an awful lot like "satisfying the urge for process and structure". Yes, code people write in professional environments should generally be satisfying user/business requirements, but that's something that can be discussed periodically, perhaps with your manager -- it doesn't seem like something that needs somebody looking over your shoulder all the time.

Ticketing systems can be useful tools, especially in environments with lots of user-submitted requests, but I'm suspicious of trying to tie every single piece of work to a ticket. That seems rather like a tool which should be your servant becoming master.

Edit: if you can't have a sensible conversation, potentially with a not-especially-technical person, about what value you've created in the past few months, then you might have a problem. One of my worries about ticket-focussed environments is that it's possible to focus on the small-scale and lose sight of what you're creating.

[blub]

The point of the tickets is to have traceability from requirements down to the code. In some industries, this is mandatory so they can satisfy e.g. safety standards.

It's a form of documentation.

[dasmoth]

Sure, it's process which is mandated in some fields.

I still think it's net harmful, and can lead to over-emphasising the small/local vs understanding the application and its value as a whole.

In cases where "you really need to understand requirement OBTUSE-1234 before you even think about touching this function" really does apply, a comment in the code addresses this much better, and will be seen by future developers even if they aren't religiously reading the VCS blame output for everything they edit (or if it's been clobbered by reformatting or other changes).

[beefheart]

A lot of these points are highly subjective and therefore dangerous points of friction. If you're going to start having a discussion about the "business need" when some presumably competent developer needs something they have already written merged, there's going to be a problem and it's not the code in question.

Again, I'm presuming competence. Code reviews can help with tutoring new arrivals, but at some point they need to graduate to a responsible, self-directed actor. At that point, chances are their code reviews will get signed off with a superficial "lgtm" anyway.

[falsedan]

This is mostly to make it easier to understand why the hell someone did something in a particular way and if it’s safe to change it to a more contemporary style years later. The reviewer should trust the author and the business requirements that lead to the change being created, and just check that it’s linked to a ticket that would make sense to that future maintenance programmer.

[blub]

They're a tool. If they're used to their maximum potential they can improve quality and increase knowledge in the team.

I am experienced and welcome reviews. They give me the opportunity to see if the team understands what I wrote or if it still needs to be adapted.

When reviewing I check for readability, to see if architectural constraints are respected and also offer advice on design and coding idioms.

[badlucklottery]

>In code review, most developers will not do the kind of thorough analysis that uncovers serious bugs, if they actually did, code review would be too costly.

The point isn't to go in-depth pick apart the design (if a dev needs that kind of deep intervention on a regular basis, it's time to have some hard conversations), it's to share knowledge. The reviewer gets to see a another dev's code along with a good description of what that change does so if there's an emergency someone on the team might actually know where to look. And the submitter gets to know of any footguns the reviewer has seen in similar code.

>Instead, disagreements on superficialities and bike-shed arguments are promoted, often leading to low-value follow-up changes that can easily cause worse issues than they solve.

Which is why it's important to have a system where the submitter can choose the reviewer(s) to avoid bike-shedding and also choose to ignore the reviewers' comments. That's Google's system. Is someone throwing a lot of BS nits at you? Ack the comments, take them off the reviewers list and pick someone else. Do that a couple times and most folks get the message. Talk to management about those that don't.

[beefheart]

> The reviewer gets to see a another dev's code along with a good description of what that change does so if there's an emergency someone on the team might actually know where to look.

In an emergency, you need developers to be able to figure things out regardless of whether they have seen something in a code review or not. Knowledge of a code base is built much more efficiently by actually working on it and, to a lesser degree, reading the code. With the time you save not doing reviews, you can let people write tests or do refactorings to build that exact skill directly, instead of having it be a side-effect.

[bcyn]

You may be underestimating how effective this type of knowledge sharing is. Knowing where to look is half the battle.

[tigershark]

I completely disagree, if I could have a penny for a bug that I found in a code review I’ll be rich.

[Too]

Completely disagree also. Around 50% of all patches I review have real logic bugs in them. Patches I write myself aren't flawless either and often someone finds bugs in them as well.

And it's not like you have to spend several hours on thorough analysis either. Small changes takes a few minutes, medium changes 30min and larger changes should deserve a few hours.

If your team is bikeshedding in code reviews, install an automatic code formatter.

[sheepmullet]

> In code review, most developers will not do the kind of thorough analysis that uncovers serious bugs, if they actually did, code review would be too costly

We replaced code reviews of every commit with in-depth code walkthroughs.

Basically whenever we start having a lot of issues with a subsystem we organise up to five half day sessions to go through the entire subsystem line by line as a group.

It brings team progress on other areas to a standstill while we do it but works way better than normal code reviews.

[a_imho]

Personally I'm at the other end of things, I routinely ask people to look at my code multiple times even before submitting it for code review. Chances I could learn about some edge case I did not consider before.