A bogus study on code review

What you need is a respected conflict mediator, probably a team lead. This person should chat offline with both authors and reviewers who are frequently involved in conflicts, for instance because they often refuse to adopt the team's idioms as an author, or because they are often on a counter-productive power trip as a reviewer. If the team, or the company as a whole, doesn't have a person who can effectively mediate this sort of thing, that's a major problem, and something the management structure needs to be keeping an eye on as a major risk to the company.

I am now a believer in pair programming. Code review is already done during development time.

The problem has always been inconsistent - some code reviews are worth thousands of times more than others - and a determining factor may be how invested the particular reviewer is in that piece of code, how well they understand the surrounding ecosystem and things of that nature.

As a concrete example, it has always been rare that a backend API change PR doesn't get great feedback from the frontend team that is dependent on it. The visibility is a core benefit here and the clear process to collaborate (especially if done early, low cost with individual small changes) is essential to the return on time investment.

For the record, I agree, I think a strong culture of code review is the most valuable process I've added to any development team. In my grandparent post, I was merely drawing attention to a potential spot for improvement, I hope no one took it as against code review as a process.

This is only for stuff like missing whitespace or a typo in a comment. It is not worth the roundtrip to call that stuff out and I've never seen it cause a meaningful conflict on my teams to simply fix it. I've only directly managed teams <10 local people or <5 remote people, I can imagine more structure is necessary on considerably larger or more disconnected teams.

While not a problem I've ever really handled, this approach may have the side-effect of reducing dysfunctional codebase ownership that I've heard about from other team leads.

For christ sake, no. If you want communicate new best practice, do so. And if team agrees, we will follow. Don't ambush me with new expectations during code review and don't keep my task hostage in order to push for your new best practice.

Same with teaching. Teach proactively instead of pretending juniors can solve it and then rewrite all their decisions in code rewiew.

If you have expectations, tell them in advance. It is just unfair to use code review for that.

I agree, but sometimes, when one has been looking through the code for a while, they can miss things that will immediately stand out to someone else who hasn't looked at the code recently.

For example, we had one developer on our team add a item to a JSON config file list and, since it was thought to be trivial, it didn't go through review. It turns out that they forgot to add a comma to the end of the quoted string. When it was time to roll out the change, we discovered the issue.

We have since enabled post-push linting on that repo, but I think that had someone else looked over it, they may have spotted that issue before it was merged.

> Again, what you are saying is right, but I believe you get decreasing values over time as the developer starts to "get" how the org wants things done.

To some extent, you're correct, but only if the code base is relatively static and conventions don't change. That usually doesn't hold true over time.

The code review I do is subtly different in those two contexts. Within my team, my goals are to keep us to a very high standard of committed code, it should be clear, simple, and well documented. We want our output to be relatively reusable, well tested and testable, and well designed. This goes beyond our newly written code, we are stewards of libraries and tools, and we should maintain them as well as we can.

On the other hand when reviewing google-wide code, I am, yes, looking for bugs, but mostly looking for good "mastery" of python. That is, I'm explicitly looking for idiomatic python from people who are not as experienced with it. These reviews also are often larger and longer.

Within my team, we have a number of practices that help make code reviews fast:

- Most changes are small: within my team I reviewed 3 changes of more than 250 lines this quarter, compared to a dozen or so of <10 lines

- Most of the code written is really good. I rarely, if ever, have to mention lint errors, nor do I often come across bugs.

- My comments mostly revolve around idiomatic ways of solving problems and, as a team python expert, "pythonic" approaches, and better approaches using the python standard library and known tooling (ie. "You're reinventing the wheel here, just use XYZ", or "this is confusing, use pathlib.path instead.")

- The other big thing I comment about is clarity and readability, as a reviewer, I want to be able to follow your code, code is written more than it is read, and so most of my comments are things that amount to "This is difficult to follow as someone reading, can you refactor this to make it easier to read." These comments vary in both necessity and type, they can range from "hey maybe reorder your conditionals here and see if it reads better" to "holy wow it took me 10 minutes with a repl open and asking for help to figure out what the heck this function is doing, this needs to be refactored."

- And the last thing I suggest is future work to be done: "We should refactor this method in the future, we're having to work around it a lot", or a very common one: "We can parameterize these tests".

- The other big thing we do to make code reviews quick is that everyone does them. They don't just fall on the TL or manager or whatever. I review code for everyone, and everyone reviews my code, and as a result the code is not mine but the team's. Nitpicks aren't really a thing, because if someone is picky just to be picky, they'll get the same treatment in turn (and also because that's a dick move and we don't do that).

Note that for the most part, the things I'm looking for are pretty easy to notice, unidiomatic code is obvious, and "this is difficult or tricky" is also very clear. I still notice bugs and such, but they're rare, and we write code to make reviewing easy and errors obvious, and as a result have fewer of them.

The across-google reviews are on average bigger than within my team, and often result in more comments, because the writers are less good at python (everyone on my team has python readability, whereas the people I'm reviewing for python do not, otherwise they wouldn't need my review). They take longer. Many of them are still relatively small, but yeah the big ones that cross 500 or 1000 lines, and are in an unfamiliar part of the codebase, those do take a significant chunk of time to review (normally ~1-2 hours), but they're rare, I get one of those every couple of weeks.

In general I'd say the answer to how to make code reviews go faster is

    1. Encourage small changes when possible
    2. Use code review to teach best practices
    3. Encourage writing code for readability and testability
    4. Have everyone review code

Edit: Added a fourth thing

Someone sends me something to review and I see a notification pop up with a summary, regardless of what I'm doing at the time. More than 90% of the time (maybe more like 98%), it is from one of my team members, who has sent out something I knew they were working on because we had chatted (in a meeting, or at our desks, or on IM) about the need for it, and often also the approach they're planning on taking.

A lot of times (maybe around 50%?) this is configuration, documentation, or a very small code or test change. These I immediately skim for obvious problems like typos or confusing phrasing, check a diff of the config for those changes to make sure it does what it thinks it does, and almost always immediately approve the change with no comments. These often take less than 5 minutes.

Maybe 10% of the time, it's a fairly small functionality-preserving refactor. I defer these until I have some free time, because they aren't critical. I review them with an eye toward making sure I think they actually preserve functionality and decrease complexity and/or confusion. These can take 15 minutes to an hour, but aren't very disruptive because I can do them when I'm not otherwise busy.

Maybe 30-40% of the time, these are medium to large functionality changes to the part of the codebase I am intimately familiar with (again: the vast majority of these are coming from members of my team). Depending on what I'm doing at the time, I'll either start reviewing it immediately, or take a note reminding myself to take a look when I get a chance. I actually start by spending some time reading the description and thinking about whether I think what it's claiming to do is even the right thing to do; if not, I comment on that before even reading the code.

After that, I dig into the code. I start by reading the whole thing at a pretty high level to see if it seems to do what the description says, focusing on method names and signatures and especially on documentation.

If all looks well at the high level, I like to start going through the changes "outside-in", that is, starting with the higher-level methods and digging into the lower-level ones over time. Mostly what I'm looking for is whether everything makes sense to me when I read it, if not I ask for renaming, restructuring, or just more documentation if it seems fundamentally complex or confusing. In other words, I try to read it from the perspective of someone coming in cold and trying to figure out what's going on. This will be the case for everyone reading this code in the future, including me and the author in a few months. I also try to think hard about what assumptions are being made, whether they are right, and what should be done about it if I'm unsure.

After all this, I look at the tests. I start by checking for good coverage. If some things appear uncovered, I ask for tests, or suggest ways it could be tested if it seems tricky. If the coverage is good, I'm honestly kind of lazy about reading the tests themselves, looking for obvious ugliness, but mostly assuming that if they're passing and covering the branches, they're probably fine. Sometimes I suggest integration tests that I think might be useful.

If the change is in code that has lots of downstream clients, I'll usually do another pass focused on whether we might be breaking them somehow. Sometimes I have to do research on libraries that are being used or could be used instead of something else.

This can all take 15 minutes to maybe two or three hours of work time, sometimes spread across a few days of wall time, including any back and forth. I wouldn't say I go "line-by-line", though I do think I end up reading all the (non-test) lines at least once. I do feel I should completely understand what's going on, but that's usually built into my experience with the code in question.

It does sound like you may be involved with more brand new systems than I usually am. My feeling about those is that it's helpful for an author to make their changes in very small pieces, so that it is as obvious as possible to a reviewer. I also think the description of the change is very important in those cases, so that the reviewer understands the purpose and approach of the new functionality. If I were to guess from your brief description, I would say it is possible you are reviewing things in larger-than-ideal chunks.

In general, though, it is definitely fairly expensive to do code reviews, but I think it improves quality and maintainability to an extent that makes it worthwhile.