Situation: code review takes too much time. Solution: announce unprecedented layoffs of 10000 programmers. Resolution: no work to be done. code review team on schedule.
11,000 employees, about half technical.
Improving code review time
41–50 of 233 posts
Re: Improving code review time
#42Earlier quoted context omitted.
> I'd say local code quality is generally good Have you ever used facebook.com? At least the frontend is incredibly slow (on a thousands of € machine), so that's not synonym with quality in my experience.
From reading the comment thread I would say the front end would be macro, local would be like a specific function on the front end, like the friends list, that by itself could have "good code".
Like when I have a red number indicating I have a notification and then I click and it just loops forever on some grey animation because it doesn't manage to load the list of notification on my fiber connection?
Re: Improving code review time
#43If you'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't catch everything. Runtime bugs that are not caught by the pipeline tend to be edge cases that don'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.
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/CD pipelines, but at the ground level most companies seem to favor adding more humans whenever the technology is not good enough.
Re: Improving code review time
#44I'm a strong believer in fast reviews.
I get into deep working mode for 3 hours a day total, on a good day. The rest is meetings, daily sync, coffee, lunch, "hey can you look at something", hallway conversations, emails, my own inability to concentrate when I'm not feeling it.
I'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.
I don't get to do deep work, at least I'm going to unblock other people as fast as possible. That means out of an 8-hour work day, you're going to get a review from me within half an hour in most cases.
I've been on the team for years and I have write access. I WILL merge your change if you pass my review.
I find that this has immense benefits:
1) People just do things. They don't schedule design meetings, get approvals, get consensus. You know why? Because if someone has a good reason why the commit wasn't a good idea, we roll back. No harm, no foul. And guess what? It happens once in 100 commits. (If it'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/testing requirements, not whether it's the right thing to do.)
2) People write good commit messages. If your commit message isn't in the following format:
Push foos in bar order instead of baz order.
Following discussion with johnsmith, benchmark
(http://) shows 12% improvement in the hot
frobnication flow.
Ticket: http://tickets/
3) People write small commits. Got a bigger change? I'll ask you to split it up (without breaking the build if we ship a version between commits). People don't push back on that, because they know it's not going to add a lot of overhead.
4) In the same spirit, people don't push back on changes I request - unless it'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 "I wanted to get this in today and I don't want to have to restore context tomorrow morning".
The velocity you reach is amazing. True serendipity. Unless you're consistently able to get full days of deep work in, I suggest you try it.
(edited for formatting)
Re: Improving code review time
#45Am 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.
If the average change is small, it's not a particularly challenging thing to take 5 minutes to review something. (for context, my median change, both mailed and reviewed, is under 50 LoC, though this will depend on language, e.g. Java is notably more boilerplate-y than the languages I usually use, and I do a lot of configuration changes that are 1-3 lines by nature). Picking up a 5 minute review after you've returned…
Yeah if it's a change to code you're deeply familiar with, that's not much of a context switch. In that situation, working on your own stuff or reviewing someone else's code feels basically the same, unless one of you is doing something very wrong.
Re: Improving code review time
#46Meta: > At Meta we call an individual set of changes made to the codebase a “diff.” GitHub: > Pull request Amazon: > Change Request GitLab: > Merge Request Google: > Changelist Nitpicking, but jesus christ, why can't we stick to a single term?
These are all just terms for slightly different perspectives on the same thing. Like diff is the summary of changes in a commit. the person who wants to affect the change submits a pull request. Another person reviews the pull request and decides whether or not to merge their code into the main codebase, etc. It's all the same thing from different perspectives.
Re: Improving code review time
#47Earlier quoted context omitted.
Are there companies with a reputation for code quality?
Google places a pretty high emphasis on code quality and readability. It's not universally great, but it's a big part of the culture. You can catch a glimpse in their [style guides][1], [abseil totws][2] and [aips][3]. Almost every change is required to be reviewed for "readability" in addition to functionality. This can feel like a lot, but it leads to pretty consistent style across the codebase which makes it a lot…
Oh, Abseil is new to me. Thanks!
Do you know where the missing tips are? For example:
The root page (parent's [2]) mentions this one as famous by name:
>Often they are cited by number, and some have become known simply as “totw/110” or “totw/77”.
Is totw/110 some Google secret sauce that we are forbidden from knowing or did they skip some weeks?
Re: Improving code review time
#48Here'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 quite agree with the proposition you are making here. It goes to the very heart of the matter, does it not. The better you are at giving detailed, explicit and concrete feedback about each and every particular aspect of a diff (NOTE: both good and bad), the better and more competent you clearly are, and the more of a true champion for the cause you are proving to be. Time and time again, I wish my reviewers would just lay it all out on the table. As opposed to the limited and perfunctory
LGTM
Re: Improving code review time
#49Re: Improving code review time
#50If you'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't catch everything. Runtime bugs that are not caught by the pipeline tend to be edge cases that don'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 pass…
Catching bugs is only one of the reasons code review is important. It is also important to transfer knowledge between developers and review design, architecture, scalability, and performance concerns.