30 ms·

CodeFlow: Improving the Code Review Process at Microsoft (2018)

166 points by rammy1234 8y ago

yodon 8y ago
I'm sure I'm not the only person who wishes VS Code had capability like this (which isn't to say everyone needs it or wants it, just that when it's needed and wanted it sounds amazing)
- humanrebar 8y ago
  You could have VS Code plug into a system like this, but there is infrastructure that is needed to run CI, track statuses, manage comment data, etc. Theoretically if there were a standard for managing all that data within the git source graph, you could implement this solely as a VS Code plugin.
- WorldMaker 8y ago
  The GitHub Pull Requests extension for VS Code is really neat. It inlines a lot of the PR workflow into VS Code. https://marketplace.visualstudio.com/items?itemName=GitHub.vscode-pull-request-github https://marketplace.visualstudio.com/items?itemName=GitHub.v... The extension is open source and the extension points it uses are all relatively well documented. I'm almost surprised there aren't more extensions doing similar things. (I don't believe that even the Azure Repos extension currently supports an inline PR experience like this.)
m0zg 8y ago
What y'all really want is Google Critique. You just don't know it yet, because you haven't experienced it, as it's internal to Google. The closest public product that I could find is http://reviewable.io http://reviewable.io (no affiliation) which, while it looks weird/amateurish, nails most of the points which make Critique so amazing. There's also Gerritt, but it's too invasive in terms of workflow, whereas Reviewable integrates with GitHub in a more lightweight fashion.
- piotrkaminski 8y ago
  Reviewable author here -- thanks for the compliment! Redoing the UI is on the to-do list, once the migration from Angular to Vue is done.
  - m0zg 8y ago
    Thank you for putting it together! I switched folks over and we've been paying customers for well over a year now. I can overlook the UI deficiencies when I have proper review workflow, but the problem is that people who don't know the Critique workflow might be put off by the UI. Personally, I encourage them to give Reviewable a good try anyway. Anything larger than a few dozen lines and/or needing multiple rounds of feedback is just untenable in plain GitHub. I can say with certainty, our code would be worse if it wasn't for Reviewable.
    - piotrkaminski 8y ago
      Thanks again, I might have to quote you on the home page. :) And yes, the UI is often the sticking point for adoption. I don't know if it's just the style / toolkit (an old version of Semantic UI) and switching to something like Material Design would take care of the problem, or if it's a deeper design / UX issue. A good onboarding flow would likely help as well but I haven't figure out what that should look like yet either. More feedback and advice is always appreciated -- and if you happen to be (or know) a UX expert who knows how to fix it I do have some budget I could throw that way. :)
      - osel 8y ago
        From my brief play with your demo it's a bit of both. The toolkit is very small and lacks good definition of elements (both visually in that it lacks contrast, and practically in that I have no idea what is clickable or editable). The page structure isn't that clear either, where are the column headings? And on my widescreen browser window the isn't a limit on horizontal stretch - I have to play with the browser to find a good width. Also, I can expand code, but how do I shrink/hide it?
pianoben 8y ago
(former Microsoft employee here) CodeFlow's workflow was nice, but everything else about it was _awful_. Capturing iterations, comments, threads, etc was the good stuff. The UI - eye-bleeding and amateurish. Network operations - terrible. Despite all that, the workflow was so good that I'd take it over github CRs in a heartbeat.
- eyjafjallajokul 8y ago
  For all of the people reading the article and trying to visualize the tool, can you summarize what about the workflow was so good?
  - samirahmed 8y ago
    Contrasting to Github PR its a better process but really only a marginal impact on developer productivity. When i was there - Codeflow had the following feature - Multiple reviewer states ... accepted/reviewing/awaiting changes/decline to review/reject change. - Multiple comment states ... pending, won't fix, fixed etc. - Iterations based code review flow - after you review - you come back easily view the next iteration (relative diff) - you can really compare back and forth and track comments through iterations - github tends to loose comments through commits - Support for bots to comment like stylecop etc Codeflow workflow felt like an MS-Office application like Word or Outlook compared to Github - Heavy UI based workflow with no API/commandline counterpart sometimes - Desktop app - not website - Lots of knobs/states - Very easy to work back and forth in the UX I agree with an earlier comment however - the UX was pretty ugly - the workflow account for lots of states Github doesn't but there is a nice linear flow to the progression of a github conversion that (at least at the time i was there) . IIRC it was a managed .NET - WPF application which are kindy clunky - not as nice as a website with clear links etc.
    - pianoben 8y ago
      At least as of November 2018 (when I left), everything you said was still true. However! CodeFlow was also integrated into VSTS, whose review tooling is very much inspired by CodeFlow. Reviewers could use their web browser or the client app, and it all fed into the same system. Most of the features that set it apart from Github initially have been copied by now - comment threads with resolutions, individual reviewer states, etc, have all been recently introduced; the gap between CF and GH has shrunk substantially. That said, CF still does a better job of retaining comment threads and associating them with lines of code. Maybe it's just different UX decisions by the product teams, but it makes a difference when reviewing complicated patches.
deleted 8y ago
[deleted]
mark_l_watson 8y ago
Good article. I liked the analysis of how code reviews are performed at Microsoft (yes, computer science is a science!). My team is fairly happy with github enterprise and the PR workflow but some things about CodeFlow sound better: being able to comment on a single character in a line or share comments between disparate code; and, having a local (non-web) client that runs faster.
dqpb 8y ago
Differential is a pretty great open source code review tool: https://www.phacility.com/phabricator/differential/ https://www.phacility.com/phabricator/differential/
- Game_Ender 8y ago
  That is sort of underselling it, Differential has the following features in common with CodeFlow: - Easy to see the changes an author makes over time to a review - Integrates with the Package and Ownership system to automatically assign reviewers - Easy work on changes that touch many files - Inline lint and code coverage results - Built in display of unit test and CI results and overall coverage % - Draft mode to delay review until tests & lint are clean - Comments move properly as you make changes - Easy to mark comments as done and find outstanding ones - Comment on multiple lines instead of a single one Things that are missing: - No ability to comment on single characters - Cannot mark certain files as important - No way to explicitly mark a change as "simple" (like a rename) - No web UI to make quick fixes (compared to GitHub is what is most missed)
alexeiz 8y ago
Since this thing is coming from Microsoft, I really doubt the tool or the whole process can work outside of Microsoft. It's likely very much tailored to Microsoft needs and full of idiosyncrasies of the Microsoft development workflow. Here's an example of how things usually are at MS. I worked at MS awhile ago. At that time Git was just being developed, and MS used it's own VCS based on Perforce (which is pretty good, I should say for a centralized VCS). Code reviews were done over email. Kind of like Linux kernel development. If you wanted somebody to review your code, you used a tool to pack your changes into a *.cmd file which contained a diff itself and a Perl (!) script to unpack and view the diff. Then you sent this file to a reviewer over email, and he just ran it on his machine to see the changes you had done. This tool was simple, limited in features, and brilliant. It was hacked by somebody on the Windows NT team back in the 90s. Even if you couldn't unpack a diff into your repository checkout, you could still open the pack as a text file and review the changes, or manually apply them, since it was just a freaking text file. Come the time MS decided to migrate everything to .NET. Some team at MS similar to TSE responsible for development tools decided that the old Perl tool was inadequate. You don't need no stinking Perl scripts! They developed a replacement in C#. Full of features you can't live without and maybe even an integration with the dev environment (I don't remember). The new tool was distributed in the binary form. The code changes for review were packed into a binary format with plenty of metadata in addition to the actual diff. The problem? Only this tool knew how to unpack this binary format. It worked most of the time and when it worked it was great. But when it didn't work, you were screwed. If it couldn't unpack your code change for any reason, it would throw some C# exception. You couldn't fix the exception and rebuild the tool (you didn't have access to the tool source code or knew how to build it). You couldn't extract the change yourself, since it was packed in a binary format only the tool could extract. You pretty much lost your change if you didn't store it elsewhere. And this is generally how things were (and still are?) done at MS. They have teams of project managers analyzing and spec'ing out every bit of the development process. Then they create extensive tools to support it with elaborate UIs and automation of everything they can think of. But if you need to do something just a little bit differently, you're left out in the cold because those tools are too close and too brittle to adapt to your specific needs.
- WorldMaker 8y ago

166 points by rammy1234 8y ago

I'm sure I'm not the only person who wishes VS Code had capability like this (which isn't to say everyone needs it or wants it, just that when it's needed and wanted it sounds amazing)

humanrebar 8y ago
You could have VS Code plug into a system like this, but there is infrastructure that is needed to run CI, track statuses, manage comment data, etc. Theoretically if there were a standard for managing all that data within the git source graph, you could implement this solely as a VS Code plugin.
WorldMaker 8y ago
The GitHub Pull Requests extension for VS Code is really neat. It inlines a lot of the PR workflow into VS Code. https://marketplace.visualstudio.com/items?itemName=GitHub.vscode-pull-request-github https://marketplace.visualstudio.com/items?itemName=GitHub.v... The extension is open source and the extension points it uses are all relatively well documented. I'm almost surprised there aren't more extensions doing similar things. (I don't believe that even the Azure Repos extension currently supports an inline PR experience like this.)

m0zg 8y ago

What y'all really want is Google Critique. You just don't know it yet, because you haven't experienced it, as it's internal to Google. The closest public product that I could find is http://reviewable.io http://reviewable.io (no affiliation) which, while it looks weird/amateurish, nails most of the points which make Critique so amazing. There's also Gerritt, but it's too invasive in terms of workflow, whereas Reviewable integrates with GitHub in a more lightweight fashion.

piotrkaminski 8y ago
Reviewable author here -- thanks for the compliment! Redoing the UI is on the to-do list, once the migration from Angular to Vue is done.
- m0zg 8y ago
  Thank you for putting it together! I switched folks over and we've been paying customers for well over a year now. I can overlook the UI deficiencies when I have proper review workflow, but the problem is that people who don't know the Critique workflow might be put off by the UI. Personally, I encourage them to give Reviewable a good try anyway. Anything larger than a few dozen lines and/or needing multiple rounds of feedback is just untenable in plain GitHub. I can say with certainty, our code would be worse if it wasn't for Reviewable.
  - piotrkaminski 8y ago
    Thanks again, I might have to quote you on the home page. :) And yes, the UI is often the sticking point for adoption. I don't know if it's just the style / toolkit (an old version of Semantic UI) and switching to something like Material Design would take care of the problem, or if it's a deeper design / UX issue. A good onboarding flow would likely help as well but I haven't figure out what that should look like yet either. More feedback and advice is always appreciated -- and if you happen to be (or know) a UX expert who knows how to fix it I do have some budget I could throw that way. :)
    - osel 8y ago
      From my brief play with your demo it's a bit of both. The toolkit is very small and lacks good definition of elements (both visually in that it lacks contrast, and practically in that I have no idea what is clickable or editable). The page structure isn't that clear either, where are the column headings? And on my widescreen browser window the isn't a limit on horizontal stretch - I have to play with the browser to find a good width. Also, I can expand code, but how do I shrink/hide it?

pianoben 8y ago

(former Microsoft employee here) CodeFlow's workflow was nice, but everything else about it was _awful_. Capturing iterations, comments, threads, etc was the good stuff. The UI - eye-bleeding and amateurish. Network operations - terrible. Despite all that, the workflow was so good that I'd take it over github CRs in a heartbeat.

eyjafjallajokul 8y ago
For all of the people reading the article and trying to visualize the tool, can you summarize what about the workflow was so good?
- samirahmed 8y ago
  Contrasting to Github PR its a better process but really only a marginal impact on developer productivity. When i was there - Codeflow had the following feature - Multiple reviewer states ... accepted/reviewing/awaiting changes/decline to review/reject change. - Multiple comment states ... pending, won't fix, fixed etc. - Iterations based code review flow - after you review - you come back easily view the next iteration (relative diff) - you can really compare back and forth and track comments through iterations - github tends to loose comments through commits - Support for bots to comment like stylecop etc Codeflow workflow felt like an MS-Office application like Word or Outlook compared to Github - Heavy UI based workflow with no API/commandline counterpart sometimes - Desktop app - not website - Lots of knobs/states - Very easy to work back and forth in the UX I agree with an earlier comment however - the UX was pretty ugly - the workflow account for lots of states Github doesn't but there is a nice linear flow to the progression of a github conversion that (at least at the time i was there) . IIRC it was a managed .NET - WPF application which are kindy clunky - not as nice as a website with clear links etc.
  - pianoben 8y ago
    At least as of November 2018 (when I left), everything you said was still true. However! CodeFlow was also integrated into VSTS, whose review tooling is very much inspired by CodeFlow. Reviewers could use their web browser or the client app, and it all fed into the same system. Most of the features that set it apart from Github initially have been copied by now - comment threads with resolutions, individual reviewer states, etc, have all been recently introduced; the gap between CF and GH has shrunk substantially. That said, CF still does a better job of retaining comment threads and associating them with lines of code. Maybe it's just different UX decisions by the product teams, but it makes a difference when reviewing complicated patches.

deleted 8y ago

[deleted]

mark_l_watson 8y ago

Good article. I liked the analysis of how code reviews are performed at Microsoft (yes, computer science is a science!). My team is fairly happy with github enterprise and the PR workflow but some things about CodeFlow sound better: being able to comment on a single character in a line or share comments between disparate code; and, having a local (non-web) client that runs faster.

dqpb 8y ago

Differential is a pretty great open source code review tool: https://www.phacility.com/phabricator/differential/ https://www.phacility.com/phabricator/differential/

Game_Ender 8y ago
That is sort of underselling it, Differential has the following features in common with CodeFlow: - Easy to see the changes an author makes over time to a review - Integrates with the Package and Ownership system to automatically assign reviewers - Easy work on changes that touch many files - Inline lint and code coverage results - Built in display of unit test and CI results and overall coverage % - Draft mode to delay review until tests & lint are clean - Comments move properly as you make changes - Easy to mark comments as done and find outstanding ones - Comment on multiple lines instead of a single one Things that are missing: - No ability to comment on single characters - Cannot mark certain files as important - No way to explicitly mark a change as "simple" (like a rename) - No web UI to make quick fixes (compared to GitHub is what is most missed)

alexeiz 8y ago

Since this thing is coming from Microsoft, I really doubt the tool or the whole process can work outside of Microsoft. It's likely very much tailored to Microsoft needs and full of idiosyncrasies of the Microsoft development workflow. Here's an example of how things usually are at MS. I worked at MS awhile ago. At that time Git was just being developed, and MS used it's own VCS based on Perforce (which is pretty good, I should say for a centralized VCS). Code reviews were done over email. Kind of like Linux kernel development. If you wanted somebody to review your code, you used a tool to pack your changes into a *.cmd file which contained a diff itself and a Perl (!) script to unpack and view the diff. Then you sent this file to a reviewer over email, and he just ran it on his machine to see the changes you had done. This tool was simple, limited in features, and brilliant. It was hacked by somebody on the Windows NT team back in the 90s. Even if you couldn't unpack a diff into your repository checkout, you could still open the pack as a text file and review the changes, or manually apply them, since it was just a freaking text file. Come the time MS decided to migrate everything to .NET. Some team at MS similar to TSE responsible for development tools decided that the old Perl tool was inadequate. You don't need no stinking Perl scripts! They developed a replacement in C#. Full of features you can't live without and maybe even an integration with the dev environment (I don't remember). The new tool was distributed in the binary form. The code changes for review were packed into a binary format with plenty of metadata in addition to the actual diff. The problem? Only this tool knew how to unpack this binary format. It worked most of the time and when it worked it was great. But when it didn't work, you were screwed. If it couldn't unpack your code change for any reason, it would throw some C# exception. You couldn't fix the exception and rebuild the tool (you didn't have access to the tool source code or knew how to build it). You couldn't extract the change yourself, since it was packed in a binary format only the tool could extract. You pretty much lost your change if you didn't store it elsewhere. And this is generally how things were (and still are?) done at MS. They have teams of project managers analyzing and spec'ing out every bit of the development process. Then they create extensive tools to support it with elaborate UIs and automation of everything they can think of. But if you need to do something just a little bit differently, you're left out in the cold because those tools are too close and too brittle to adapt to your specific needs.

WorldMaker 8y ago