4

My boss say we should find a way to scale code reviews at our company. As it is right now, we have about 16 software developers spread across 4 different teams/squads, but soon the company will close a deal which will double the company size. With that, eventually we'll have dozens of developers. He recommend me checking out automated tools such as this one: https://www.freecodecamp.org/news/how-to-automate-code-reviews-on-github-41be46250712/ but I'm particularly not a fan of automated tools due to:

  • They don't understand the context to check if variable/method names are good, bad or even counter-intuitive
  • They can't judge if the chosen architecture is being followed
  • Developers can decide to ignore the machine comments (for example PyLint, even though we have it, everybody just ignores it)
  • Some of them have costs
  • Some of them (like the one I mentioned) require special access to the repository/github organization, which I don't have

We work with Python (mostly), Lua and Go, and among the developers (generally speaking at the company), even though everybody sees values in code review, most just perform a "quick view" and say LGTM, while 2 or 3 (me included) like to nag about possible bugs and improvements. So how would you guys recommend to scale code reviews? How large companies like Microsoft, Google, Amazon, Oracle, etc, perform code reviews with dozens/hundreds of developers, making people actually care about code review? Is it worth to create something like a gamefication platform/leadership board (maybe with rewards)?

3
  • 1
    Limit the number of developers involved in any one code review, and you'll be able to scale. – Erik Eidt Sep 27 '20 at 23:08
  • Code reviews are very Important and a person has to do it. Take any popular opensource project. There are thousands of people that contribute. I'd say generally the senior members of the team should do a round of code review after a round of peer review. If you think everyone is equally expeienced, a round of peer review should suffice. Always a different set of eyes should take a look at the code before it goes into production. – Vasantha Ganesh Sep 28 '20 at 14:44
  • 1
    It heavily depends on your culture, requirements and code base. Do you need all dev to review all code? Some dev? specific devs? Even in big company the only experience I have is dev are in small teams (5 person) and only work on their code base. Only 1 or 2 peer reviewer were needed. In this case there would have been no sclaing hence why I think you need to clarify your current practices and target practices – JayZ Sep 29 '20 at 7:02
6

The first thing to accept is that you cannot fully automate a code review. However, you can automate, to varying degrees, specific aspects of reviews to alleviate some pressure on the human reviewers and let them focus on the things that cannot be automated. Use a linter to find (and hopefully correct) stylistic issues. Static analysis tools can highlight security or performance issues, unused code, and other potential problems. Depending on the tools, developers can run them locally before opening a code review or they can be incorporated into your build process, making reports available to code reviewers or even cause build failures. Depending on your technologies, there are likely to be various solutions, some of which are free and open-source software and others which are commercial offerings.

Once you have automation for what can be automated, your humans can focus on things like the architecture and design of the system as well as the readability of the code. That will also take a cultural shift. A quick check probably isn't sufficient. Although tools can find some issues, humans still need to spend a good amount of time reading the code and asking questions. I have found that having higher quality code going into the review can help, it won't help change attitudes. Everyone should be aligned on what the expected outcome or purpose of a code review is for the team and the organization.

4

Mostly you scale by having everyone review a pull request for every pull request they author. The automated tools don't replace a human reviewer, but they let the human focus on more important matters.

If you're a reviewer who often has more to say in a review, you can scale by doing levels of reviews. I give a full review to anything from my own team (3 other developers at the moment). For pull requests authored by other teams, I look at the titles of everything, but anything with a routine-looking title I ignore. Some titles merit a quick glance at the pull request, and some quick glances merit a deeper review. Anything I don't review deeply, I leave to someone else to approve.

This system leaves me with one or two reviews per day outside my own team. Outside of that, yes, the occasional picky thing gets missed, but mostly you're making a judgement call about where your time is best spent.

3

Ask yourself this question - what is the point of the code review stage?

Here are some possible answers to that...

  1. To ensure a consistent style of code - Use an automatic code linter in the CI/CD pipeline and reject code commits that do not follow the project style guide. No humans required.
  2. Review the implementation approach - Too late in the process. You want to do a design review, with followup to ensure that the implementation is following the design. The code review stage happens at the end of the work so its expensive to fix implementation/design divergence.
  3. Identify Code Smells - These are identifiable by good code analysers. Implement this in the CI/CD pipeline, and fail builds that have these issues unresolved. Require all exemptions to be reviewed as part of a design review.
  4. Training / Coaching Exercise - There is some benefit in this at the point when someone has started demonstrating competence acting as the final review before completing the task, but this should only need to be a small percentage of the changes over time.

In short, there are lots of ways the human effort in code reviews can be reduced, if the company is willing to spend engineering effort to integrate tooling

2

This is an odd question.

I mean, code reviews don’t take any more time to do when you get more developers. And by definition, when you have more developers writing code you automatically have more developers capable of reviewing code.

And getting developers to care about reviewing code happens the same way it does at small companies. Some people will really care. Some people will really not care. Your culture and rewards (financial and respect) will sway the remaining plurality.

There are plenty of good enough tools to alert people and make that low friction. Adding process is more likely to cause people to rubber stamp things.

5
  • depends a bit on the form of code reviews. In some places a code review is done with the whole team or by the lead. So at the least one needs to sub-divide the big group of developers into smaller teams then and have sub-leads. But other than that, I agree, code reviews as a process scale perfectly fine. (enforcing the code preferences of a single person is harder to scale^^) – Frank Hopkins Sep 28 '20 at 3:01
  • 1
    Still, the "whole team" is your team of ~7 engineers. When you add more teams, it doesn't make the code review any slower or any less scalable. – Telastyn Sep 28 '20 at 3:06
  • hehe, yeah, my point is you first need to be "smart enough" to split up all those new developers into separate teams instead of inviting everyone in the dev department like you used to. On a "improvement suggestion" level I'd say, perhaps clarify why you think code reviews scale by themselves, i.e. perhaps add a one line how you imagine a code review to work (e.g. one person reviews another person's code, so always only two persons involved etc.) just to clarify for people who employ "weird" code review models. – Frank Hopkins Sep 28 '20 at 3:10
  • i.e. I had the same thought ("but they scale by themselves, no?!") but then thought about variations where it is not so obvious on first sight and what information a person coming with such a mindset would need. As always, take my comments just as a suggestion, feel free to adapt if you agree or well don't ;) Have a nice day/night. – Frank Hopkins Sep 28 '20 at 3:13
  • 1
    Enh? I've run into a few dozen varieties now. All of them scale fine with developers unless you do something absurd like having the entire engineering department in every single code review. And even not even the dumpster fires I've worked for would do something that dumb. My personal views on code reviews are unusual, and a longer discussion that isn't appropriate here. I feel like this answer handles the common forms (single reviewer, lead reviewer, in-person single reviewer, dual reviewers, team review, in-person team reviews). The "culture" bits is also a novel. – Telastyn Sep 28 '20 at 3:23
2

The best way to make developers care about a code review is to make them review code they might actually someday have to maintain.

The best code reviews are conducted very soon after code is written. When the author is still willing to make changes. Some make the changes during the review. This bleeds into pair programming.

If, however, you’re looking for a way to scale a conference table peer review just know that the more people you put in the room the less real communication will happen.

Code stalking, where people watch each other’s check ins using source control, will still work fine. It scales because people tend to only stalk code that they interact with. Here the best automated tool is email.

As for automated tools, my only recommendation is that it’s very nice when a check in has both a coders and a peer reviewers name attached to it.

2

Code reviews are as important as writing the code itself. Sadly, many teams see them as just a hoop in the process to jump through and don't take them seriously. I don't know about automated code reviews, but teams should always be looking for ways to do CR better, not so much faster. I don't really see why scaling is a problem. I don't think the entire team should review every line of code. Two developers, at least one senior, is more than enough to review every commit. Other best practices I'd recommend:

  • Don't just do reviews 'remotely' using Github. Sometimes have reviewers sit down with the devs and discuss the code in person.
  • Always make Code Reviews the top priority for devs. If a dev is assigned to review, they need to practically drop anything else they happen to be doing to do the review. This is because when a dev is waiting on a review he or she is effectively blocked from doing any other work, and, of course, because you want to push latest changes to production as soon as possible.
  • Encourage reviewers to actually check out the code and see it working on their machines rather than just gazing at a diff.
  • Encourage junior devs to do code reviews and respect any ideas or suggestions they have. At the very least it will help them learn the code base.
  • Don't just review the code, review the tests as well.
8
  • "If a dev is assigned to review, they need to practically drop anything else they happen to be doing to do the review " seems odd to me. I've seen a few review processes and certainly there exist some where a dev should wait for the review before starting something else, but in all processes I was involved in so far, an open review was absolutely no blocker for anyone. Reviews were always done async. You can start working on something else. If you need code from that other branch merge it into your new feature branch or cherry pick what you need. etc [cont] – Frank Hopkins Sep 29 '20 at 14:34
  • So sure they should not lie around forever, but the assertion there feels a bit too strong to apply generally. Take it as a hint that there are various review processes out there. – Frank Hopkins Sep 29 '20 at 14:35
  • I've just found that code reviews can build up if not addressed quickly, and once that happens you have problems of code rot. The response is often to rush through the backlog of reviews rather than doing so at a considered pace. Once devs start new work, they lose context on the older work, and why not push new code through the process? If it seems too strong, it's because I really can't see any reason for not doing it this way. – Richard Hunter Sep 29 '20 at 17:43
  • Well I for one don't like having to interrupt any other work, because that gets me out of the topic I'm currently working on. On the other hand, most reviews I did or got didn't bounce that much that I needed to delve into specifics again on my own stuff or the other way around. General style remarks don't require delving into the topic again and even so, the rough topic I can keep in my head a couple of days. But sure, I agree that they should be covered soonish. My baseline rule however is more "before starting something new, look if there are open reviews". – Frank Hopkins Sep 29 '20 at 17:50
  • 1
    +1 Some great stuff here - particularly about making code reviews the top priority. As I've heard it said: "Code reviews should be the #1 priority in any dev team because a) They are a source of possible bugs and b) They are unrealised value". I've had the misfortune of working with devs who leave reviews for weeks on end, causing failed sprints. Ironically, the same devs expect their code to be reviewed pretty much immediately. Code reviews should always be timeboxed. Working on something else isn't an excuse - do it first thing, after lunch or before home time. Don't be a blocker! – Robbie Dee Sep 30 '20 at 21:06
1

The key thing to get right is to document what you expect the code review to achieve and, generated from that, the rules code is to reviewed against.

This keeps the reviews tight and enables any developer to quickly review any code.

So for example you could say

Reason for Code review

  1. Ensure adherence to security policy
  2. Reduce bugs

Rules of Code review

  1. Must have unit tests (see reason 2)
  2. Must have no plain text passwords (see reason 1)

Now you can:

  • maybe automate some checks
  • quickly run through all the rules when reviewing code
  • argue whether you need more or less rules
  • resolve any differences of opinion by going back to the reasons
  • assure the business/auditors that your security policy is being enforced
  • know that code reviews are not about enforcing GDPR/Naming Conventions/whatever else

Critically arguing about what the rules should be has been separated from enforcing the rules.

If you have a plain text password then its a fail, and the fix is clear. If you have no unit tests its a fail and the fix is clear.

There's no criticism of code or discussion about whether X is appropriate for Y. That can all be saved for the Reasons and Rules meetings

0

In the same way that code generators will never replace coders, tools will never replace code reviews. It just isn't possible.

Now your boss wants to scale code reviews but I'm really guessing they just want them done faster. So the obvious question is how to limit what is reviewed and how can we make those reviews faster.


LIMITING REVIEWS

Allow Simple Changes

Not everything requires review. For a simple spelling change, you might just allow developers to check those changes in. Similarly, if an earlier review has already taken place and the change is fairly simple, you may allow developers to just to check those changes in once they're done.

Pair Programming

While pair programmed code should still be reviewed, it is likely it will contain fewer errors. Consider doing this for complex developments as a matter of course.

Have Quality Gates

Code should reach a minimum standard before it is even presented for review. Make those expectations clear to avoid review rounds (see Lean on Tooling below).

Consider Scope Of The Code

What is the scope of the code. Is it a proof of concept? Is it a quick and dirty tool that will be used once and thrown away? If it is never going to reach the echelons of production then you may want to consider a more lighter touch review.

Quicker To Run It Up?

Rather than pore over pages of code, would it be simpler just to build it and run it up in a test environment?


IMPROVING THROUGHPUT

Ensure The Process Is Equitable

The process should be fair for all. We all know reviewers who want every "i" dotted and every "t" crossed while others seem to waive everything through. There are also developers who implement every suggested change while others think they know better and will do the bare minimum. Ensure reviewers differentiate between those changes that have to be done and those which are simply a question of style or taste.

Avoid Lengthy Coding Standards Documents

If developers are having to wade through pages of coding standards, that is going to hold things up. Only document what isn't covered by tooling.

Rotate Reviewers

Rotate reviewers. It stops bottle necks and helps spread the knowledge.

Timebox Reviews

Reviews and rework happen best when it is fresh in everyone's minds. If a reviewer is allowed to park reviews for weeks on end you have a culture problem or a bottleneck (see point above).

Lean On Tooling

If code isn't even building with passing unit tests, it doesn't need review, it needs fixing. See quality gates. While tooling isn't the complete answer, it can do a lot of the menial tasks for you.

Not the answer you're looking for? Browse other questions tagged or ask your own question.