7 ms·

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 blo

by epage 5y ago

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.

londons_explore 5y ago
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.
- chii 5y ago
  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.
  - londons_explore 5y ago
    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.
- epage 5y ago
  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.
- fivre 5y ago
  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.
zdw 5y ago
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.
- bialpio 5y ago
  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...
  - wyldfire 5y ago
    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.
    - coryrc 5y ago
      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).
    - zdw 5y ago
      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/Documentation/3.4.1/user-review-ui.html#reply https://gerrit-documentation.storage.googleapis.com/Document...
  - surajrmal 5y ago
    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.
tomasreimers 5y ago
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 :)
- epage 5y ago
  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.
- SideburnsOfDoom 5y ago
  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.
  - ahartmetz 5y ago
    "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.
javajosh 5y ago
>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.
- epage 5y ago
  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.
andrekandre 5y ago
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?
- epage 5y ago
  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.
  - xapata 5y ago
    An easy fix is to have the mentee drive.
    - phist_mcgee 5y ago
      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.
      - apotheon 5y ago
        Okay, solution: teach the person to use a computer first
vogusar 5y ago
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.

by epage 5y ago

londons_explore 5y ago

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.

chii 5y ago
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.
- londons_explore 5y ago
  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.
epage 5y ago
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.
fivre 5y ago
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.

zdw 5y ago

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.

bialpio 5y ago
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...
- wyldfire 5y ago
  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.
  - coryrc 5y ago
    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).
  - zdw 5y ago
    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/Documentation/3.4.1/user-review-ui.html#reply https://gerrit-documentation.storage.googleapis.com/Document...
- surajrmal 5y ago
  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.

tomasreimers 5y ago

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 :)

epage 5y ago
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.
SideburnsOfDoom 5y ago
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.
- ahartmetz 5y ago
  "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.

javajosh 5y ago

>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.

epage 5y ago
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.

andrekandre 5y ago

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?

epage 5y ago
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.
- xapata 5y ago
  An easy fix is to have the mentee drive.
  - phist_mcgee 5y ago
    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.
    - apotheon 5y ago
      Okay, solution: teach the person to use a computer first

vogusar 5y ago

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.