Comment by epage | Hacker News Reader

Comment by epage | Hacker News Reader

At my last job, I found our code review process was terribly slow and set about analyzing it and coming up with solutions. I found a lot of developers were blocked on reviews. They either task switched (expensive) or babysat their review (expensive). Even though we were using Phab, few did stacked commits because of how unfamiliar the tools were which made them feel brittle.

Improving the stacked workflow is one part of the equation but I'd recommend being careful of letting that mask other problems

I identified a series of busy-waits including:

- Post, wait, CI failure, fix, post

- Post: wait for reviewer, ping reviewer, wait, find feedback was posted, fix, post

Some of the root causes included:

- Developers weren't being pinged when the ball was in their court (as author or reviewer)

- Well, some notifications happened, but review groups were so noisy no one looked

- The shared responsibility of review groups is everyone assumed someone else would look.

- The tools we were using made it hard to identify active conversations

My ideal solution:

- Auto-assign reviewers from a review group, allowing shared responsibility of the code base while keeping individual accountability of reviews

- Review tools that ping the person the review is blocked on when they are ready to act

- Review tools that give a reminder after a set period of time

- Azure DevOps' tracking of conversation state with filtering (and toggling between original code and the "fixed" version)

- Auto-merge-on-green-checks (including reviewer sign off)

With this, responses are timely without people polling the service, letting machines micro-manage the process rather than humans.

Gerrit does many of the "ideal solution" items, as it replicates the internal Google workflow.

It's a code review system with browsing as an afterthought, as opposed to GitHub which is a code browser with review as an afterthought.

Gerrit is nice, but to me feels a bit dated, UX-wise. Before joining Google, I used Azure DevOps and I feel that it was easier to review code over there (less jumping around the page). At first I thought it's a matter of (in)familiarity with the tool, but I still feel like I'm fighting with it after 3 years of use...

I really like that with Gerrit I'm not encouraged to force-push a PR branch. There's a couple other things that are nicer on Gerrit but GitHub/GHE is great that it has markdown in comments.

Gerrit only has force push for the PR equivalent.

The nice thing it has is really easy to rebase off other people's in-progress work (for GitHub you need to to fetch the other person's commit by url and hash -- I guess after writing that down it's not a huge advantage).

Gerrit does support some Markdown-like formatting:

     Some basic markdown-like syntax is supported which renders indented lines preformatted, lines starting with "- " or "* " as list items, and lines starting with "> " as block quotes

from: https://gerrit-documentation.storage.googleapis.com/Document...

There has been a lot of investment in Gerrit in the last few years and the UX is improving dramatically. It's still not perfect, but I've been very happy with it lately.

The Gerrit UI is just bad (confusing). I don't like the "dated" criticism for work tools because whether it works well is mostly independent of it being fashionable - cf. Steve Jobs quote "Design is how it works". Gerrit's UI is both dated and confusing, but confusing is the major thing here. For example, "+1" and "-1" meaning "I like / don't like it" and "+2" / "-2" meaning "I approve / block it". The numbers are not summed up, the names are misleading. Or the workflow of writing a comment and then publishing it. Probably makes sense when making several comments (on different blocks of code), but it's too easy to forget the publishing, especially when writing a top-level comment where there isn't anything else to publish together.

Another thing I don't like about Gerrit is that they seem to play Tetris with UI elements, i.e. "wherever there's room to put it is where we put it". It's not as bad as it used to be and improving, and it looks less dated than it used to, too. There are also newish features like patch editing in the web interface.

All that said, patch queues / stacks and the rebase based workflow are nice.

Definitely, "feels dated" is very subjective. The other problem ("I feel like I'm fighting with the tool", "I'm jumping all over the place to do what I want") may be related to your "UI tetris" point, and I may be unnecessarily tying it to the UI feeling "dated".

ISTM that what is needed is to observe how people interact with the tool and then try to figure out how the workflow can be improved based on those observations. Or maybe I'm trying to interact with gerrit in some strange way - that's also possible.

The +1 / +2 for different labels aren't that confusing to me - additionally, in Chromium I found that gerrit is very descriptive when you on-mouse-over the buttons, but IDK if it's specific to this deployment or if that aspect got improved.

Totally totally totally agree. I think a lot of the slow down comes from not understanding when the ball is in your court. And Github doesn't prioritize this b/c I think this problem is less prevalent in open source, where there is usually no expectation of a reasonable time until review.

A lot of what you described is exactly what we've built / we're planning for the code review portion of the site - you should sign up and check it out :)

Github has been improving on this. I felt like they were rolling out the features I wanted right as I was enumerating them!

They have

- Auto-assign reviewers

- Pings for people (unsure how sophisticated)

- Auto-merge

The review conversation itself is the big issue.

Agreed 100%

Making it easier to stack PRs feels like entirely the wrong direction - "when you're in a hole, stop digging" - stacking is giving the people in the hole better shovels to keep digging with, instead of getting PRs flowing through more easily so that stacking is not needed so much.

"Stop digging" is not an option when working on a project that isn't entirely internal to one (sub)organization. These are not only open source projects.

To add to your list:

> Reviewer able to make tiny changes to the code without a further round trip to the developer. Things like "missing full stop here" or "Please put a comment warning about X here". Developer should get a heads up that their code has been merged with changes, just so the tiny changes don't go unreviewed.

i'm a bit ambivalent about this sort of idea - reviewers adding "tiny" changes without extra reviews could be a place where bugs leak in. what's tiny or insignificant might turn out to be big - that's why code is reviewed by multiple people!

I think for style formatting, making it an auto-format step during commit/push is the "better" way. Codified formatting means you argue and hash it all out once and be done with it forever.

Nothing stops an organisation having standards what is sufficiently tiny to use the process...

Eg.: "Touches comments and whitespace only". "Resulting compiled binary does not differ". "Variable and function renames are okay". etc.

Remember that with the original developer still able to review, they can always rollback a change they weren't happy with.

I feel the hard part is putting automation around this.

I loved it when I worked on a small, high trust team where we were comfortable with each person making the decision on whether it was safe to do a post-merge review or that it needed attention first with a pre-merge review.

EDIT: Something I have liked at a couple of my company's was a "cowboy commit audit" where we allowed people to directly push but doing so would ping the code owners and they would have to review to clear the state. People understood they shouldn't but also understood when it happened.

Github suggestion comments usually do this pretty easily. You can prompt the submitter to accept a diff. It does have two annoying problems though:

- You must use a browser editor, no attaching a diff you generated elsewhere. Anything more than 1 line is hell because Github's editor gives no fucks about running gofmt or whatever. - If you request a change, approve the PR, and then the submitter accepts your change, it dismisses your approval.

i've heard it said that pair-programming can alleviate the need for doing code review... or at least the typical in-depth reviews that usually become bottlenecks...

any experience with that?

My primary experience with pairing was not ideal. It was being used in a mentoring capacity rather than when people are more equal. I also found that the person doing the mentoring usually drove which meant that the mentee just saw things whizzing by on the screen and had no idea what happened and when they did they didn't have the first hand experience to commit it to memory.

I do remember a situation of hot seat programming where we had a critical problem right before release that crossed domains and three of us worked at one desk consulting the others or swapping out as needed. Again, it was worth it because of the urgency and priority but not something worth doing generally.

An easy fix is to have the mentee drive.

Now navigate to the FOO service

FOO is the background event scheduler, it's connected to BAR via RPC

Ok don't worry about what that is for now, just have a look at the service logs

So it's a daemon that running on port 5000

You'll have to tail the logs in /var/foo/logs

So type T. A. I. L, and then dash f

No, the hyphen character, not literally dash

And so on and so forth. It's not that a person more senior than you can't drive, but I have found a lot of time the implicit knowledge of so many systems, and topics and accreted domain knowledge can really be a productivity drain. The best pairing usually comes from equally matched individuals.

Okay, solution:

teach the person to use a computer first

Sounds good -- I know that you're a great teacher. You'll still have those features done by tomorrow, right?

I read your parent as "that guy should not be anywhere near programming, he needs to learn how to use a computer first".

Okay I might exeggerate a bit but if someone literally writes 'dash' at that point, don't pair on something that has a deadline like that.

Do take him aside and let him know he gotta brush up on the damn basics and that you are there for him through it but you're gonna finish that feature with a deadline first while he takes a stab at that easy bug over there.

Exactly. The easy fix is not to have the mentee drive.

Tell the PM that we need to go slower to get faster.

Have you ever paired with a junior before? This is definitely the kind of thing you would encounter with a fresh grad/intern.

A software engineer at a tech company that doesn't know about commandline flags? That doesn't seem very realistic.

Anyway, this person will learn that dash means - , and not make that mistake again. It's a one-time mistake. So I think the pairing helps even in this case.

> A software engineer at a tech company that doesn't know about commandline flags? That doesn't seem very realistic.

Why not? 95% of most peoples work can be done in an IDE/text editor, and that last 5% is probably very exploratory anyway. I work in games, and gameplay programmers work almost exclusively out of IDEs with GUIs for source control and most other dev tools etc.

> Anyway, this person will learn that dash means -, and not make that mistake again. It's a one-time mistake.

Disagree that it's a one time mistake. On any given day/task there might be 5/10/15 of these. dash, ctrl + r, tab complete, !!, different shells, shebangs, flaky backend services, etc etc. If you want to teach someone how you work then yes this might be appropriate.

> So I think the pairing helps even in this case.

That said, I agree :) It depends though, is your goal to bring two programmers up to speed or is it to crunch through tasks right now? Because pairing helps with one of those goals but not the other

> the mentee just saw things whizzing by on the screen and had no idea what happened

That's a bad pairing experience, and I've had a handful of those in my last five years of pair programming.

On occasion I've said, "Look, why don't you tell me what to do, and I'll drive?"

But I think that the larger issue is that some developers are better at solo'ing than pair'ing, and there's nothing wrong with that, and at the company I worked (Pivotal), we'd find positions for the people who were more inclined to solo.

Well that's obvious, because the person who did the pairing is already pretty familiar with the code, so even if you do perform a review it will go fast.

I've had great experiences with pair programming (and also bad experiences, but this is assuming a great experience), and I'd still want code review. The benefits can actually stack, if not compound, often enough.

I suspect something missing in pair programming that you get in a code review is someone stepping back and looking at the change as a whole, rather than the incremental bits as you go along. It can be helpful! I know at least when reviewing, there are things I miss when I only look at the update vs looking at the PR as a whole.

The way I'm used to pair program is that the pairing counts as a much more intense review than the typical corporate review case.

So the whole waiting aspect is completely gone.

Sure, this can have bad results with less skilled and/or serious people. But that's always true.

Pair programming doesn't scale for all the purposes of code review. Pair programming can be great for bursts and unblocking when there is good alignment, but code review serves as an enduring artifact for the whole team including future team members.

that's like saying meetings alleviate the need for emails (documented decisions). Ideally you need both, but pair programming/meeting needs to be scheduled, with all the associated problems, async communication doesn't. If you have to pick one over another, then obviously that's async communication. A meeting without minute is a meeting that didn't happen.

Pair programming can't alleviate the need for code review. Good pair programming may reduce the amount of changes needed, and thus speed up the review phase, as a secondary effect of increased code quality.

Bad pair programming will suck up time by occupying two people at reduced efficiency, and won't lead to better code.

I've had some really great pair programming experiences, but we as a team aren't mandated to use it. We see it as a tool to pull out when cross-domain knowledge is needed, or when facing a particularly thorny problem. If you get the right two people together, they can fill in the gaps of each other's knowledge. The person not driving has more mental capacity available to think through potential alternatives.

However, the success of pair programming depends as much on the personalities of the individuals doing the pairing as anything else. They need to function as teammates, without any ego issues or any other thing that may prevent either party from feeling fully comfortable sharing their knowledge. Trying to pair program with someone who is defensive about their code/abilities is a true nightmare.

Once a review is done, it can help to sit with the person to avoid an endless back & forth via whatever app's hosting your repo. In my experience developers usually just put aside 1-2 moments per day to spend on reviews, so if there's 3 back & forths that can mean the changes don't get accepted for several days. Sometimes for just a few lines of code.

At my company, we do exactly that using Github+Slack integration. Github Codeowners + Github Teams let's you to autoassign teams as reviewers. When the team is assigned then a random person from the team is pinged using round robin. Furthermore, with Github Scheduled Reminders you can get notifications in Slack according to some schedule and/or when some event happens, e.g. your PR was approved, got a comment or a mention, etc.

>Auto-merge-on-green-checks

ADO has that.

Post, wait, CI failure, fix, post

This is the worst, especially when the CI system is homegrown, rickety, and difficult or impossible to reproduce locally.

When I was doing product evaluation at my last company, I was torn between AzDO and Github. After being burned by the lack of progress on Phab (before it went away), one of our criteria was what I called a "living platform" meaning that it either has a vision that aligns with us and progress is being made or there is a vibrant community where we can collaborate. AzDO has a lot of good features but it doesn't seem like its getting much attention and everyone has been keeping their integrations with it private, killing off collaboration.

Reply on news.ycombinator.com