How to do a code review

I can offer a personal anecdote here on my experience with code reviews, on both sides.

I was the primary architect and reviewer for a complex real-time mathematical application. When reviewing code, I was pretty much a tyrant: the code had to be correct, well tested, conform to the theory, interface with the rest of the system correctly, etc., in order to be allowed in. I remember leaving some pretty brutal reviews when the proposed design was different than what I thought it needed to be. I thought I was doing the right thing, the project lead thought I was doing the right thing, but maybe I was just making my teammate's lives hell.

In a subsequent job, I was on the receiving end: I was again the primary architect and maintainer, but still needed to seek review from a larger team (my component was part of a larger project they owned). The experience was not enjoyable: reviews sometimes took months (I sure wish I was kidding), sometimes were passthrough "LGTM! I don't understand it at all!", sometimes asking questions like "why is this mutex here" and then I have to spend 3 hours writing up an explanation for how threads and locks work in this case. I found that my mental model shifted: instead of committing small improvements here and there like cleaning up comments or renaming something I just... didn't. I didn't want to deal with a multi-day process of bugging someone to review (they were always busy), dealing with the roulette wheel of comments that might come up, the possibility that I might have to justify some minor thing that I don't even remember the reasoning for. It felt like making a PR opened you up to an uncomfortably invasive inspection, one where the reviewers look down their nose at you and ask you to elucidate why you chose to wear the red shirt today instead of the blue one, as if you're supposed to have some grand unified theory of shirt colors when the actual reasoning is "I thought red would work and it did". How are you supposed to justify why you didn't do all the things you didn't do?

I think an issue is that there's always a different approach that could be used and in a perfect world perhaps we'd iterate endlessly until we found the best one. I've seen plenty of systems that have a design very different than what I think I would do, but as it turns out those systems work too.

I've honestly become less convinced that code reviews are the answer. Is there a possibility for learning reviewer submitter? Of course. Do some teams find code reviews to be hugely beneficial? I would assume so. But I don't know if an organization-wide mandatory absolutely-zero-exceptions is the way to go.

> Google is mostly tests and it is one of the largest and most successful C++ projects in history of our industry.

I'm not quite sure what you mean here? Are you talking about Search? Ads? Cloud? Self driving cars? Google has various products of varying business and technical success levels. Some stuff I feel is a good example of quality software (let's say Go for example or maybe Maps) and some stuff seems to just be very poor software (let's say Google Hangouts Chat or Google Sheets). Some stuff is really good business and some gets cancelled (with little apparent correlation to quality).

Google is so rich that perhaps it is succeeding despite some practices and not because of them. It's initial success probably predates all these practices and there's plenty of quality software in the history of the industry that has been produced using a variety of other practices.

Even if you are right and Google is the most successful C++ code base ever that's still not evidence that some particular practice is the cause of that. There is no way to measure any of this and "feeling" doesn't really cut it. Some practices apply more to certain kinds of software and possibly less to others.

Doesn't Google use Linux? For juicy hot garbage it's done pretty well. With a budget that's a fraction of what Google has. It's also not C++.

re-EDIT: Another thing to note is that Google has a large number of engineers developing internal tools, build systems, test engineers etc. With all the automation around dealing with flaky tests and testing in general at scale perhaps this works better for Google. Other companies who do not have the luxury of having 100's of engineers work on their build or test systems may run into different problems when they try to do things the Google way. Because Google is overall fairly opaque (the open bits are exceptions) it's hard to really come to a conclusion about how well they do software. I've heard different stories from different sources and I'll bet there's lots of internal variation as well. The success as a company is undeniable but there's more to that than software.

No, the point is Google goes beyond the typical organization in that it provides 20% time which can, if you desire, be used to work on personal projects even with Google equipment. That's better than using after-work hours and your own resources.

How would I go about writing a test for an extremely rare race condition between two components 10 levels of indirection away covering 10s of thousands of lines of code? Because I've fixed bugs like that but can't fathom a way to write a test for it, I only even found the cause by grepping 6 months worth of logs and finding a recurring pattern.

I briefly considered writing a debugger script to pause at the right spots so QA could replicate the issue but would be a waste of everyone's time and I wouldn't want that in an automated suite.

The point of reviewing code is to get a second set of eyes on it, partly for perspective and partly to keep you honest. Pair programming is almost strictly better.

The tradeoff is that you've now effectively made the reviewer a full-time participant in the development process, which is much more of a resource drain than a code review role would have been. This may end up being a very wise investment, but smarter people than me have strong opinions on both sides of that one.