[wellpast]

What often goes unmentioned in praise for code review processes is their insanely exorbitant costs -- measured in engineer hours but perhaps more costly is all of the blocking and impedance [1]. Most of the "problems" that code reviews claim to address can be solved by much more direct and optimal measures. Code reviews are damn expensive.

This post concedes that code reviews are better for the more fluffy ends -- teamwork, openness, social recognition, but given their high costs, I'd rather achieve even these soft goals in other ways than to impede my team's delivery potential.

While mission critical systems deserve the whole kitchen sink thrown at them, expensive verifications, code reviews, etc etc., most business applications would do much better optimizing for better software architectures and domain conceptualization than spend so much time dwelling on the minutiae of lines of code.

[1] Continuous integration and refactoring, pillars of agility, go out the window in typical code review environments where commits are blocked until peer review.

[geophile]

Amen.

Another huge cost of code reviews is distraction. We've all seen the Paul Graham essay on maker's schedules vs. manager's schedules. We've all read the statistics on how much time is lost to interruptions. Code reviews are a massive interruption, done on a manager's schedule. Each code review is a distraction, and can take a significant time commitment, if it is to be a meaningful review.

A few years ago, I worked at a company with a strong emphasis on code reviews, and it turned into a waste of time when people couldn't afford to waste time. You'd have to do the review, but you really needed to get back to your development or bug-fixing because of the impending hard deadline, so there would be non-commital non-review reviews such as "looks good to me", or "I see no problems". While a good code review can be valuable, these perfunctory ones are a huge waste of time.

Finally, code reviews sometimes substitute for design review, which catches the most serious problems much earlier. At this same company, it drove me crazy that we always had time for code reviews, but never for design reviews.

[devishard]

> Another huge cost of code reviews is distraction. We've all seen the Paul Graham essay on maker's schedules vs. manager's schedules. We've all read the statistics on how much time is lost to interruptions. Code reviews are a massive interruption, done on a manager's schedule. Each code review is a distraction, and can take a significant time commitment, if it is to be a meaningful review.

Wait why are we doing code reviews on the manager's schedule? Why not just wait until you're finished a task to do them?

[geophile]

What? And keep the other developer waiting?

[phasmantistes]

Code reviews should be asynchronous. Everyone on your team should be able to be working on multiple small changes/commits/whatever in parallel. While one is out for review, they're working on the others. Different people keep different schedules: I do all my reviews first thing in the morning, to settle in, and then often do another round after lunch. Other people do them at the end of the day, or don't mind the interruptions (e.g. follow a pomodoro schedule anyway) and do them as they come in.

[koja86]

Context switching also creates some overhead. YMMV as the time it takes to switch tasks IMHO depends on particular person, context scale and problem difficulty.

[devishard]

This has nothing to do with code review. If you're finished with a task, you're going to have to switch contexts; you can't just keep working on something that's done.

[devishard]

Waiting to do what? Why can't they just go off and do something else?

[qb45]

> Finally, code reviews sometimes substitute for design review, which catches the most serious problems much earlier. At this same company, it drove me crazy that we always had time for code reviews, but never for design reviews.

You reminded me of one cargo-scrum team working like that.

Important design decisions were made thoughtlessly on "sprint planning" meetings since they were required for division of the "story" into "tasks". Then the classic "garbage in, garbage out" law guided the sprint.

[nvarsj]

> What often goes unmentioned in praise for code review processes is their insanely exorbitant costs -- measured in engineer hours but perhaps more costly is all of the blocking and impedance [1].

I don't get why you think code reviews are so expensive. An engineer should rarely be blocked by a code review. If waiting for a review - don't. Pick up another task! A great thing about code reviews is they are done async.

If the cost is time spent doing the review - what's the alternative? No code review at all? That's like not testing - yes it's faster now, but you'll pay for it dearly later on. (Code review is well studied in academia / industry, and has consistently shown to be very effective. Having at least one extra pair of eyeballs on any code goes a long way.)

[geophile]

As the comments all over this thread demonstrate, code review is done for many reasons. Where I've worked, the reasons have not been clearly stated, and as a result there is a lot of time wasted. I wouldn't say that there should not be code reviews, but they could be cut way down. A code review should not be a substitute for a design review. It often is, in my experience. A code review should not be done for anything that can be done by static code analysis: code style, finding dead code, etc.

Also, a lot depends on how code reviews are done. I've seen them used as a veto, with lots of back and forth to get the veto removed over subjective and trivial issues. Don't do that.

Finally, let's be honest. Some people's code doesn't need review. Time spent reviewing such code is wasted. But it is often socially difficult to say this, so everyone's code gets reviewed.

Other people can't be trusted to write acceptable code, and reviews are essential. That's a different problem. (Beyond the scope of this note. Left as an exercise to the reader. Other cliches may apply.)

[renku]

What are these "more direct and optimal measures" that are cheaper than code reviews?

In my experience code reviews are much cheaper than other means of raising software quality. For example unit tests only start to add real value after you have written a good bunch of them, so they form this regression-safety-net.

I haven't seen Continuous integration and refactoring being thrown out of the window because of code reviews. Quite the opposite. Code reviews usually ask the author to refactor his code further. Similarly I have no idea why one would stop continuous integration because of code reviews.

[wellpast]

Continuous integration means you can work on multiple items at once and that you break up large feature dev into smaller chunks that get committed to master. For example, I might commit the data layer for a new big feature well before the UX, etc.

Code reviews mean that every time I want to integrate with master I've got to 'grab a lock' and schedule and wait for a code review.

What typically happens in these code review cultures is you tend to make monolithic commits (to minimize the # of code reviews you have to do) and you also tend to minimize diffs with the code base (to make the CR easier for the reviewer).

[phasmantistes]

What? These statements and conclusions make no sense to me.

Code reviews mean that every time I want to integrate with master I've got to 'grab a lock' and schedule and wait for a code review.

You're using a terrible code review system. You should be able to have multiple potential commits out for review at any given time. You should also have multiple pending commits from your coworkers on your queue at any given time. The code review process is asyncrhonous: process them at your leisure (though sooner is better than later, for obvious reasons). Once the review has been approved, it can be rebased onto or merged into master. If the patches you're writing are so far-flung that they don't apply cleanly, break them into smaller pieces.

What typically happens in these code review cultures is you tend to make monolithic commits (to minimize the # of code reviews you have to do) and you also tend to minimize diffs with the code base (to make the CR easier for the reviewer).

These two statements are contradictory. Monolithic commits by definition increase the diff with the codebase, not decrease it. What you tend to see are meaningful commits: commits which do one specific thing, and fully test it, without breaking any other systems.

[renku]

The problems you describe aren't really problems with Code Review per se, rather with the process that's enforcing the reviews.

The idea of code reviews is that somebody else reads your code and provides feedback. Whether the reviewer is sent a pull request, a list of commits in master or a printout is a matter of implementation.

I see a cyclic problem in here: Reviews take lot of time -> developers want to get more done, not wait for a review -> so they send more changes to review -> reviews take even more time -> ...

I think the solution is to find ways to make reviews smaller instead. Breaking tasks to smaller pieces that take less time to implement and review. (But not by reducing the # of lines in diff - squeezing several changes into one commit is a sure way to make it harder to review.) Smaller reviews will also be more effective - with large reviews there's a tendency for a reviewer to wear off and just skim through the changes.

Of course that's easier to say than do... it's something I'm trying to do by myself and not fully succeeding.

[gracenotes]

I do occasionally ask newer employees who are not experienced with code reviews to split up a review into multiple smaller reviews. Actually there is a natural equilibrium that happens here. There is an exponential relationship between complexity and size of a single change and review latency, just because that's most efficient for reviewers (assuming some hard limit on acceptable latency as well). Most changes take on a uniform medium size as a result of this. The sheer number of changes isn't as big a factor here because most reviewers look at reviews in batch anyway.

Separately, minimizing diffs with the codebase is not just easier for the reviewer, it also makes 'blame' and resolving problems with existing code easier. You shouldn't break your back to minimize diffs, but as a form of hygiene it is not without merit.

Your definition of continuous integration is interesting in that I haven't seen it used in that way, or rather with that emphasis. Smaller syncs-to-master than "this is the entire feature" are great, but their effectiveness depends on automated testing, building, and deploying. When code under review is a coherent, high-quality, and tested thing, this can be a boon for CI because you get a small chunk with clear before and after states, which you might not get from just winging it. You can also work on code while other code is under review, especially with a Scrum/Kanban-type task system.

[solipsism]

I'm curious what you mean by grab a lock and schedule and wait for a code review. I've used all sorts of different SCMs, code review software, in-person code review processes, etc.. but I've never heard of anything like that. Could you describe your process to us in more detail?

[zelos]

I haven't done it myself, but I know of teams that use a lock for integrating changes to the main branch.

Without it the rate of change in the branch was fast enough that submissions would be preempted by someone else's and you'd have to rerun presubmission tests etc.

[jupp0r]

If anyone is ever blocked, you are doing it wrong. When performed on git branches, you can easily branch off you current feature branch, keep going and rebase to master when the original branch has been merged. Continuous integration is with you all the time and helps the reviewer by annotating the diff with code coverage, cyclomatic complexity and other metrics. A good review culture emphasizes design feedback over low level issues. In addition, plenty of positive feedback is as important as finding issues.

[kerkeslager]

> What often goes unmentioned in praise for code review processes is their insanely exorbitant costs -- measured in engineer hours but perhaps more costly is all of the blocking and impedance [1]. Most of the "problems" that code reviews claim to address can be solved by much more direct and optimal measures. Code reviews are damn expensive.

I'm on a team where for about a year, part of our process has been that code doesn't get merged until three people say it's ready. If one person wrote it, it needs two reviews, or if a pair wrote it, it needs one review.

Your mileage may vary, but my experience with this is that the code reviews increase latency, but don't really have much effect on bandwidth. It might mean that there's a few hours between finishing your code and merging it into master, but during those hours you can be doing other things. Most code reviews take <10 minutes, and I usually just check for other people's PRs when right after I create a PR of my own, so it doesn't interrupt my flow.

Sometimes PRs do get blocked, but in those cases it's usually for good reasons; the design has some serious flaw or is very overcomplicated, or there's a bug. I see that as similar to a failing build. Sure, a CI server sometimes blocks you from merging, but do you really want something that fails tests or doesn't compile going into your master branch anyway? Code review is just like having a CI server that builds and runs tests, but it checks things that can't be automated.

We do sometimes run into cases where code review takes a long time, but the root cause there is usually that we didn't break down the feature into an adequately small enough minimum viable product. 1000 lines of changes takes more than 10 times as long to review as 100 lines of changes; the increase isn't linear. If code review is taking a long time, it may be an indication that your continuous integration isn't continuous enough.

That said, "code review is 100% necessary" and "code review is 100% too costly" are just different kinds of Kool-Aid you shouldn't drink. "People and interactions over processes and tools"[1] applies here. What works for my team might not work for yours. I'm not arguing for code review; there's no universal right way to create software. I'm arguing that code review is an effective process step for some teams and an ineffective one for others.

[1] http://www.agilemanifesto.org/

[r-s]

I have a hard time imagining that most code reviews take < 10 minutes.

* Understanding of the problem at hand, reading through the story, understanding the context of the the code change.

* Pulling down the code, reading through the commit log

* Reading the specs, possibly running coverage tools if not part of testing suite.

* Understanding the logic of the code, seeing if the tests cover the edge cases

* If dependencies change, they may need to be investigated too

* Thinking if there are better ways make code a bit more extensible or understandable

* Thinking of risk of the code (security, performance, deployment concerns)

* Reading through associated documentation and ensuring its accuracy

Doing all of this takes me longer then 10 minutes always.

[skybrian]

That sounds like a more thorough review than we're talking about here. It's more like following a link to a website, reading the diffs, and adding inline comments. And it's typically done by someone already somewhat familiar with the codebase.

Also, some code reviews are tiny changes, so it brings the average down.

[kerkeslager]

* We mostly understand the problem at hand already because we were in the same planning meeting.

* We do our code reviews on GitHub by looking at the diffs. No need to pull code.

* We review the code of the tests, but we don't run them. Our CI server runs tests and coverage tools.

* The code is usually a short MVP, <100 lines including tests, so understanding logic and edge cases doesn't take long.

* If dependencies change, it was a group discussion and we already investigated.

* Again, small commits make this quick.

* Small commits make this quick.

* Documentation is handled by our product manager.

I guess what I'm learning from reading your comment is that part of why our code reviews are fast compared to yours is that we mean drastically different things when we say "code review".

[CorvusCrypto]

I feel like this can be argued so many ways. For instance, while code review might block some changes from being introduced into the system, it also does have more benefits than the "fluffy" ones you mentioned. Maybe an engineer that has more experience with the codebase can point out a way of doing something that is more in line with what the team's style is (earlier move toward standardized code vs letting people do what they want). Maybe you missed a utility function that already achieves what some of your code does. Maybe there's not a bug, but again a more experienced dev sees something that can be optimized and will save time in the future that would be spent on tracing system performance issues. The idea that we need to move ultra fast as devs... I think that is necessary in some cases, but I'm not convinced that taking some time to code review tasks is as expensive as you suggested. I mean even reviewing 1000 lines of code should take no more than 30 minutes. And if it looks entirely confusing and hard to follow, then maybe it's something to revisit because I guarantee you're gonna be spending longer if you have to go back and refactor/extend messy and unclean code that technically works. I think the key here is to catch things that stand out and not review the minutae.

This is just my opinion of the matter. On the topic of team building, I don't really care for all that stuff. Unit tests w/ CI and periodic refactoring are great for revisiting design decisions. Code reviews are great for catching bad design decisions early on, though so use it as a tool.

[wellpast]

So the problem you're talking about is how to properly manage (and coach) less experienced devs or devs who write messy code. While code reviews can tangentially deal with this, I think there are much more direct approaches to these problems than an umbrella code review policy.

[simplify]

What direct approaches do you have in mind?

[draw_down]

Yes, our peer review mostly consists of looking through the diff to make sure nothing seems crazy or out of place, stray print statements, etc. Every single pull request can't go through a gauntlet of close examination, we wouldn't get anything done.

[knorker]

> insanely exorbitant costs

wat?

Either the code is easily readable and correct, and takes barely any time at all to review, or it's buggy and detecting this early rather than late saves a yuuuuge amount of time.

Also, the reviewer now knows the code. So in reviewing the code they've already half way to being able to improve the code in the future.

And the reviewer can also pick up "oh, that's a neat pattern". Code reviews are education for both parties. Are you saying companies should not spend time or money on education/courses?

And as an author I really appreciate not only that someone looks for me having made a mistake, but because I know someone will read the code I won't go with the easy way. I can fool myself, but I can't fool someone else. E.g. the right thing to do is to name this constant or make an enum, but I'll just put a literal "4" here because I know it's 4. (which of course I won't remember in a week).

[ArkyBeagle]

1) Commits don't have to be blocked. The action items from a review can be a separate commit.

2) Reviews can be done privately, with comments entered electronically. There doesn't have to be a four-day meeting.

2a) Organizations that use four-day meetings are likely full of people who need something to charge that time to. It may be worth delegating a representative from the engineering team to attend those.

3) Action items must be triaged. Ideally, each action item would be assigned a number and numbers grouped together as a statement of work for a commit. This is much less painful than it sounds. Test provenance would also be nice per commit.

[eduren]

>This post concedes that code reviews are better for the more fluffy ends -- teamwork, openness, social recognition, but given their high costs, I'd rather achieve even these soft goals in other ways than to impede my team's delivery potential.

What techniques have you found effective for improving the soft goals in the context of software (genuinely curious)? Are we talking more conventional management/business concepts or something a bit more loosely defined?

[wellpast]

Not sure how to formalize but the team's I've worked on have generally negotiated mutual respect, openness, and teamwork by collaborating on the things of greater import -- the architecture, domain conceptualization, etc. And peer code reviews in some cases tend to work against these goals--because you tend to be down in the weeds of LOC, bike-shedding, arguing over the equivalents of tabbing and spacing or whether a line could be more functionally expressed, e.g....

[thrownaway2424]

Well perhaps another part of Google's code review culture is important, then: proposed changes must be style compliant, must compile, must have tests, and all the tests must pass before anyone will bother looking at them. If I got a code review (at Google) that was incorrectly indented, there would be a little red chip in the review UI that indicated such style violation, and I would just reply "Please fix" and not look at it again until the next revision.

In the absence of a strong style guide and testing culture then I agree code reviews can get bogged down in bikeshedding. At Google that doesn't seem to happen because the style guide for each language is quite prescriptive. For Go, no change will be reviewed that hasn't passed through gofmt, and so forth. And on the flip side you can have confidence that if your change has gone through gofmt, there's not going to be any discussion about the formatting.

[alxndr]

What UI does Google use for code review?