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.
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?
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.
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.)
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.)
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.
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).
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.
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.
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.
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.
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.
* 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.
Also, some code reviews are tiny changes, so it brings the average down.
* 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".
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.
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).
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.
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?
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.
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.