Improving code review time

&gt; Next reviewable diff<p>Holy Fuck No.<p>90% of my PR review time goes into &quot;Okay, how will this change impact parts of the system that this mid-level Dev doesn&#x27;t understand yet?&quot; and &quot;Does this PR actually do what the ticket it is claiming to implement actually intended?&quot;<p>Reviewing diffs in isolation completely removes one&#x27;s ability to do that.<p>If you remove a person&#x27;s ability to do that, what you&#x27;ve left them with is the easiest part of the PR, just checking that the logic seems logical. And honestly, most of that work can be automated by linting, style cops, and unit tests.<p>The fact that they got rid of the part of the PR review process that matters, and only saw a 1.5% improvement speaks to all sorts of problems in the process overall, not an improvement by this tool

Am I reading this right? If the total median time in review is “a few hours”, that means that people are dropping what they’re doing to review the code. That can’t really be a good code review? A context switch alone would be half of that time for me.

I&#x27;m a strong believer in fast reviews.<p>I get into deep working mode for 3 hours a day total, on a good day. The rest is meetings, daily sync, coffee, lunch, &quot;hey can you look at something&quot;, hallway conversations, emails, my own inability to concentrate when I&#x27;m not feeling it.<p>I&#x27;ve been in this industry for coming up on ten years. None of this is going to change, unless I become an academic or a hermit.<p>I don&#x27;t get to do deep work, at least I&#x27;m going to unblock other people as fast as possible. That means out of an 8-hour work day, you&#x27;re going to get a review from me within half an hour in most cases.<p>I&#x27;ve been on the team for years and I have write access. I WILL merge your change if you pass my review.<p>I find that this has immense benefits:<p>1) People just do things. They don&#x27;t schedule design meetings, get approvals, get consensus. You know why? Because if someone has a good reason why the commit wasn&#x27;t a good idea, we roll back. No harm, no foul. And guess what? It happens once in 100 commits. (If it&#x27;s something truly complex, you do get a design doc approved first. But then the review is about making sure your code is correct, matches your design and our style&#x2F;testing requirements, not whether it&#x27;s the right thing to do.)<p>2) People write good commit messages. If your commit message isn&#x27;t in the following format:<p><pre><code> Push foos in bar order instead of baz order. Following discussion with johnsmith, benchmark (http:&#x2F;&#x2F;&lt;shorturl&gt;) shows 12% improvement in the hot frobnication flow. Ticket: http:&#x2F;&#x2F;tickets&#x2F;&lt;ticket_number&gt; </code></pre> I&#x27;m sending it back to you. Since I merge most of my team&#x27;s code most commits look like that.<p>3) People write small commits. Got a bigger change? I&#x27;ll ask you to split it up (without breaking the build if we ship a version between commits). People don&#x27;t push back on that, because they know it&#x27;s not going to add a lot of overhead.<p>4) In the same spirit, people don&#x27;t push back on changes I request - unless it&#x27;s for a good reason. Discussions are on-point. When changes are made you get back approval half an hour later. No background psychological pressure of &quot;I wanted to get this in today and I don&#x27;t want to have to restore context tomorrow morning&quot;.<p>The velocity you reach is amazing. True serendipity. Unless you&#x27;re consistently able to get full days of deep work in, I suggest you try it.<p>(edited for formatting)

there’s so many low hanging fruits for improving the quality of diff viewing. The worst code reviews are often the ones where code get refactored, leading to piles of delete &#x2F; create lines that are just code being moved or slightly renamed.<p>One very simple approach would be better git integration with the IDE, helping build commit that make sense, where a set of changes could easily be commented by the author as they’re performing the edits, then keep improving from there.

When I was there, you could always just put<p><pre><code> Reviewed-By: self </code></pre> in the commit, and not wait. Much faster ;)

&gt; Next reviewable diff<p>As the commit author, it&#x27;s in my (and everyone&#x27;s) interest to size changes up so that it&#x27;s easier for review, and also present in them in the logical order of thinking. Personally, I prefer the bottom-up approach.<p>I bring the non-functional and impertinent changes (like refactoring and tangential changes) ahead in the line-up so that the actual changes are kept separate and are concentrated at the tail end.<p>I make commit messages of the pattern: Present situation, the problem with that, what this patch does, and what the effect it has&#x2F;how it solves the problem or sets up a path forward.<p>The initial PR might be sliced too thinly, and so will have more commits than ideal. But, as the review progresses, and once both the reviewer(s)&#x27; and the author&#x27;s mental models are in sync, commits can be collapsed at their logical boundaries.<p>Regardless of the tooling and presentation, it&#x27;s imperative that that the reviewers are intuitively aware of the ramifications of the change. Without that, the review ends up being nit picking, spell checking, and whatever that&#x27;s obvious on the immediate vicinity, and the process degrades into a box-ticking exercise.<p>No AI needed. Be human.

Somewhere in the post, it&#x27;s mentioned fb uses code ownership logic in the next review engine.<p>If folks are interested, there&#x27;s project called <a href="https:&#x2F;&#x2F;github.com&#x2F;milin&#x2F;gitown" rel="nofollow">https:&#x2F;&#x2F;github.com&#x2F;milin&#x2F;gitown</a> which does something similar in github leveraging code owners.

Here&#x27;s another factor at Meta that can reduce code review time: Your performance review is based in part (maybe not a large part, but in part) on how many reviews you perform, and how many words you put in to those reviews. edit: In short, people are incentivized to review

I came to the conclusion long ago that mandatory code reviews are a waste of time. For critical stuff, absolutely.<p>But PRs and review cycles over burden dev teams and don’t seem to move the quality needle higher one bit.<p>A better way is to ensure multiple hands touch a given area of the code, so that multiple eyes ultimately are seeing and manipulating those bits. If they are given a task to do in that area they will be motivated to understand it (and potentially improve it). By contrast, with code reviews often the reviewer does not have time to really deep dive into the code and will only have a superficial understanding of it.<p>Oh, also use code quality scanners to keep an eye on tactical code debt.

In the spirit of tangentialism I randomly suggest: Architecture Review!<p>- Prevents juniors from being blown out of the ocean into startalloverland by seniors at tail end<p>- Focus on the most dangerous aspects of the change that can&#x27;t be fixed later<p>- Sets the stage for more informed programming reviews later on (lower priority to me though)

Have I missed the feedback from the users? There should be some quotes from team members who liked the change. Their mentioning that they start being data-driven for internal tools suggests that they start treating developers like cattle and not pets.<p>&gt;Driving down Time In Review would not only make people more satisfied with their code review process, it would also increase the productivity of every engineer at Meta.<p>This hasn&#x27;t been tested. &quot;The average Time In Review for all diffs dropped 7 percent&quot; - they have verified that they changed the left side of the equation, the review time, but they haven&#x27;t checked the outcome, the productivity. Overall it doesn&#x27;t seem like they have checked if their changes have negative side effects.<p>Likewise<p>&gt;The choice of reviewers that an author selects for a diff is very important. Diff authors want reviewers who are going to review their code well, quickly, and who are experts for the code their diff touches.<p>doesn&#x27;t match<p>&gt;A 1.5 percent increase in diffs reviewed within 24 hours and an increase in top three recommendation accuracy (how often the actual reviewer is one of the top three suggested) from below 60 percent to nearly 75 percent.<p>They have shown that the people they nudge are more likely to do a code review. But are they the experts who do the review well?<p>The 1.5 percent in reviewed diffs could also be jitter.<p>*edit: Meta could extend the review process. There doesn&#x27;t seem to be a review process for the review team. If they don&#x27;t like to review their changes, or if they cannot find suitable reviewers, how are they qualified to role out their changes to the software development team?

If you&#x27;re going to add machines to the process why not add it with the purpose of eliminating the human from the process all together? Reviews are necessary because compilers and linters can&#x27;t catch everything. Runtime bugs that are not caught by the pipeline tend to be edge cases that don&#x27;t happen until there is enough data to test (in the general sense) the feature. ML could be used for smart testing and if it passes the code diff merges automatically.<p>It always surprises me how much software companies want to rely on human verification. The whole point of programming is to automate and let the machine take care of it. Every few years the industry does add new tools to automate process like CI&#x2F;CD pipelines, but at the ground level most companies seem to favor adding more humans whenever the technology is not good enough.

On a much smaller scale (with a team of 8), but I also noticed this problem and wrote a “nudge bot” for Slack and Gerrit. It takes the team-relevant changes and post it to a Slack channel in a formatted message with the patch state (not reviewed, pending, needs change, etc)<p>I made a talk about it, unfortunately in Hungarian, but you can see screenshots how it worked: <a href="https:&#x2F;&#x2F;youtu.be&#x2F;7WiICWyP1sQ" rel="nofollow">https:&#x2F;&#x2F;youtu.be&#x2F;7WiICWyP1sQ</a><p>Here is the code:<p><a href="https:&#x2F;&#x2F;github.com&#x2F;kissgyorgy&#x2F;slack-review-bot" rel="nofollow">https:&#x2F;&#x2F;github.com&#x2F;kissgyorgy&#x2F;slack-review-bot</a>

Is it just me, or in the last couple of weeks (since announcement of layoffs) there&#x27;s been an increase in FB tooling&#x2F;infra threads? Could be Baader–Meinhof effect, of course.

Stacked diffs are awful compared to PR&#x27;s. Not every commit should be clean

If code is important enough, it will get reviewed and tested one way or another.<p>Everything else is a waste of time.

All these comments about how code review is a waste of time, or suggest code review is only for bugs, really shine light on why so much software is incredibly slow today.

Too many think code reviews are an opportunity for endless debates over personal preference. A code review should be fast and cover blatant good practice violations and architectural mistakes. Everything else should be taken care of by linters and tools. If a reviewer wants code done in a different way they can write the code themselves.

Meta:<p>&gt; At Meta we call an individual set of changes made to the codebase a “diff.”<p>GitHub:<p>&gt; Pull request<p>Amazon:<p>&gt; Change Request<p>GitLab:<p>&gt; Merge Request<p>Google:<p>&gt; Changelist<p>Nitpicking, but jesus christ, why can&#x27;t we stick to a single term?

Disclaimer: these are anecdotal reports. I&#x27;ve heard from a lot of my friends how abysmal the quality of code is at Meta. Obviously, this may not be true in all teams&#x2F;products, but that&#x27;s the general sentiment. Why make it faster when you&#x27;re already dealing with mess! This is abundantly evident from the constant fire fighting, duct tapes and a metric driven culture that incentivizes the number of diffs landed.

Situation: code review takes too much time.<p>Solution: announce unprecedented layoffs of 10000 programmers.<p>Resolution: no work to be done. code review team on schedule.