{"id":435,"date":"2017-09-22T04:27:25","date_gmt":"2017-09-22T09:27:25","guid":{"rendered":"https:\/\/blogs.msdn.microsoft.com\/mobilecenter\/?p=435"},"modified":"2019-07-05T10:58:34","modified_gmt":"2019-07-05T15:58:34","slug":"how-the-visual-studio-mobile-center-team-does-code-review","status":"publish","type":"post","link":"https:\/\/devblogs.microsoft.com\/appcenter\/how-the-visual-studio-mobile-center-team-does-code-review\/","title":{"rendered":"How We Do Code Review"},"content":{"rendered":"<p><em>Visual Studio Mobile Center is now Visual Studio App Center. <a href=\"https:\/\/blogs.msdn.microsoft.com\/vsappcenter\/introducing-visual-studio-app-center\/\" rel=\"noopener noreferrer\" target=\"_blank\"><em>Learn more here<\/em><\/a>.<\/em>\n&nbsp;\nThe Mobile Center team is a diverse mix of engineers, product managers, and UI\/UX designers with varying professional backgrounds, experience, and countries of origin. Our team has people from over 10 countries working across 8 time zones. Some are fresh out of college, while others have been in the industry for more than 20 years, and we all came in with our own understanding and definition of \u201ccode review.\u201d<\/p>\n<p>We spend a significant part of our day doing code review in GitHub and <a target=\"_blank\" href=\"https:\/\/www.visualstudio.com\/team-services\/\" rel=\"noopener noreferrer\">Visual Studio Team Services<\/a>, so we welcome anything that makes it faster, easier, and better. After pinpointing several common pain points, we established and <em>widely circulated<\/em> a common set of code review principles for submitters and reviewers.<\/p>\n<p>I\u2019m sharing how we\u2019ve reduced friction and, most importantly, have more time to do awesome work. Any team can use what we\u2019ve learned to improve <em>their<\/em> code reviews, regardless of size or end product.<\/p>\n<h2>Common Issues<\/h2>\n<h3>Issue #1<\/h3>\n<p>One of the biggest causes of frustration? Pull requests that sit open, in limbo for days or weeks. Pull request lags result in:<\/p>\n<ul>\n<li><strong>Bottlenecks<\/strong>: Long-running pull requests block people from continuing with their work, especially in situations where many others depend on the code being merged and deployed (e.g end-to-end integration tests).<\/li>\n<li><strong>Wasted work<\/strong>: As pull requests age, they become stale and out of sync with upstream branches unless the submitter regularly rebases or merges upstream changes while waiting for the initial review.<\/li>\n<li><strong>Uncertainty<\/strong>: Open and unfinished pull requests add mental load for submitters; they need to constantly check status, make updates, and be on alert for reviewer comments or change requests.<\/li>\n<\/ul>\n<h3>Issue #\u00a02<\/h3>\n<p>Pull request comments easily become a rabbit hole, drifting off and focusing on a small, irrelevant area. In our experience, this is due to differing opinions between submitters and reviewers, not objective facts, and results in a stalemate.<\/p>\n<h2>The Golden Rule of Code\u00a0Review<\/h2>\n<p>Before diving into reviewer and submitter-specific guidelines, keep in mind one common, critical rule:<\/p>\n<blockquote><p>Respect others, yet don\u2019t take anything personally! We all make mistakes at some point. What\u2019s important is to learn and improve, and to treat others how <em>we\u2019d<\/em> like to be\u00a0treated.<\/p><\/blockquote>\n<p>Everyone\u2019s opinion matters; some might be biased, some might be outdated, and some of them are problematic in a particular context. It doesn\u2019t matter. We work together, communicate openly, self-check our feedback, and try to be as objective and fact-based as possible.<\/p>\n<p>As a submitter, remember that reviewers have their own tasks, deadlines, and improvements to implement. They\u2019re taking time away from their work to help <em>you<\/em> make your code better.<\/p>\n<p>As a reviewer, remember what it\u2019s like to be in the submitter\u2019s shoes and offer honest, prompt, actionable, and detailed comments.<\/p>\n<p>Following \u201c<a target=\"_blank\" href=\"http:\/\/blog.steveklabnik.com\/posts\/2011-08-19-matz-is-nice-so-we-are-nice\" rel=\"noopener noreferrer\">Matz is nice so we are nice<\/a>\u201d has also never hurt!<\/p>\n<h2>Tips for Code Owners and Reviewers<\/h2>\n<h3>Provide clear, obvious contribution guidelines.<\/h3>\n<p>Don\u2019t make contributors wait until they get review feedback to learn basic guidelines, such as style requirements, test requirements, and so on.<\/p>\n<p>Document your guidelines in a clearly visible spot, like your Readme, and update it frequently, especially when things change, or many contributors raise the same questions. This clear documentation extends to things such as your general architecture, where to look for re-usable code, and support infrastructure (e.g. Make, Rake, or Gulp tasks to automate things). Why? This gives new code contributors an easier entry point, so they get up and running faster and don\u2019t have to spend cycles trying to figure out how to implement certain functionalities.<\/p>\n<p>Clear contribution guidelines set contributor expectations from the get-go, simplify adding changes, and result in better code overall. If it\u2019s easy to get started, you\u2019re more likely to get more (and more frequent) contributions.<\/p>\n<blockquote><p>Helpful advice for how to navigate the codebase and get started faster will very likely earn you an extra gold star \ud83c\udf1f!<\/p><\/blockquote>\n<p>As a submitter, it\u2019s frustrating to see your code sit for undefined periods of time. Document when and how quickly the team reviews code, so submitters know what to expect.<\/p>\n<h3>Don\u2019t criticize the submitter, point out flaws in the\u00a0code.<\/h3>\n<p>We\u2019re told not take it personally, but being told that you\u2019re wrong\u2014that something isn\u2019t right with the code you\u2019ve spent hours on\u2014isn\u2019t a great feeling. Don\u2019t criticize the person submitting the code, point out issues or technical problems. You\u2019re there to act as another set of eyes, provide a different perspective, and notice changes that cause unwanted side effects. Is the code unnecessarily complex? Do you see performance issues? Point those out!<\/p>\n<p>The language you use matters, so be mindful about how you phrase your comments. For example, we don\u2019t use \u201c<strong>you<\/strong> made a mistake\u201d opting instead for phrases like, \u201cIf <strong>we<\/strong> leave this as-is, <strong>we<\/strong> might run into [rationale or possible undesirable outcome] problem down the road,\u201d or \u201c<strong>This line<\/strong> could cause us trouble because\u2026\u201d<\/p>\n<h3>Automate all the\u00a0things<\/h3>\n<p>We\u2019ve learned that we prefer being reprimanded by a robot over having our teammates call out our indentation style violations. Automatic linters, like <a target=\"_blank\" href=\"https:\/\/rubocop.readthedocs.io\/en\/latest\/\" rel=\"noopener noreferrer\">Rubocop<\/a>, <a target=\"_blank\" href=\"https:\/\/github.com\/StyleCop\/StyleCop\" rel=\"noopener noreferrer\">StyleCop<\/a>, <a target=\"_blank\" href=\"https:\/\/palantir.github.io\/tslint\/\" rel=\"noopener noreferrer\">TSLint<\/a>, and <a target=\"_blank\" href=\"https:\/\/eslint.org\" rel=\"noopener noreferrer\">ESLint<\/a> are a code reviewer\u2019s best friend; simply run them automatically during the build process to catch obvious flaws early.<\/p>\n<p><img decoding=\"async\" src=\"\" alt=\"Let robots (like TSLint) catch easy mistakes and focus on giving more valuable feedback.\" width=\"800\" height=\"327\" class=\"aligncenter size-full wp-image-1076\" \/><\/p>\n<p>More advanced tools (we like <a target=\"_blank\" href=\"http:\/\/danger.systems\/js\/\" rel=\"noopener noreferrer\">Danger<\/a>) catch things that aren\u2019t directly related to the code. For example, I\u2019ve picked up that someone forgot to update a Changelog, if the pull request impacts critical pieces of the codebase, or vice versa when a pull request fixes documentation typos or other trivial changes.<\/p>\n<h3>Be a role model, and focus on what\u00a0matters.<\/h3>\n<p>It\u2019s easy to ignore your own rules, but the reviewer who expects high test coverage shouldn\u2019t commit untested code.<\/p>\n<p>As you review code, realize you may make similar \u201cmistakes\u201d that you point out in others\u2019 contributions, and don\u2019t obsess over minor style details. If the code doesn\u2019t obviously violate a major style rule, there\u2019s no reason to force a style\u2014let the committer use their preference.<\/p>\n<p>We apply the same rules to all contributors, ensuring we\u2019re not overly critical, and we hold every team member accountable for practicing what they preach. We note obvious flaws, readability issues, or hard to follow code, which allows our teammates to quickly understand and address big-picture issues, rather than getting mired in the details, and focus on delivering a better user experience.<\/p>\n<blockquote><p>Feedback should be actionable and concrete, not overly opinionated and theoretical.<\/p><\/blockquote>\n<h3>Watch your\u00a0tone.<\/h3>\n<p>Tone is important, and one of the first things to go when you\u2019re in a hurry, trying to finish another review, and just want to get back to your own code. But, how we communicate is tremendously important and makes a big difference (even more so for distributed, international teams like ours!):<\/p>\n<ul>\n<li>\u201cI have a bad feeling about this line of code\u201d isn\u2019t a well-rounded argument.<\/li>\n<\/ul>\n<p><em>Instead:<\/em><\/p>\n<ul>\n<li>Explain what could go wrong and <strong><em>why<\/em><\/strong> the code needs to change.<\/li>\n<li>Communicate your context:<\/li>\n<\/ul>\n<blockquote><p>\u201cWe used a similar pattern in the past and it failed because of\u00a0X.\u201d<\/p><\/blockquote>\n<blockquote><p>\u201cI recently read a blog post warning against this. Here is the link\u2026\u201d<\/p><\/blockquote>\n<blockquote><p>\u201cHere\u2019s something that helped me in the past\u2026\u201d<\/p><\/blockquote>\n<p>Engineers are problem solvers; that\u2019s what makes our job fun, but don\u2019t try to force <em>your<\/em> solution.<\/p>\n<ul>\n<li>Point out problems. Ask \u201chow could we make this better?\u201d <strong>not<\/strong> \u201chere\u2019s how we should fix X.\u201d<\/li>\n<li>Ask clarifying questions rather than immediately concluding that the code is wrong.<\/li>\n<\/ul>\n<h3>Introducing the Emoji\u00a0Code<\/h3>\n<p>Tone is complex enough, but written feedback makes is difficult to convey <em>meaning<\/em>. After a few misunderstandings about what certain comments meant, we implemented the Emoji Code\u2122*. While it\u2019s not a substitute for written feedback, we incorporate emoji as an indicator for the kind of comment we make whenever we can.<\/p>\n<p>Our most used Emoji and their \u201cdefinitions\u201d:<\/p>\n<ul>\n<li>\ud83d\udc4d or `:+1:`: This is great! It always feels good when somebody likes your work. Show them!<\/li>\n<li>\u2753 or `:question:`: I have a question \/ can you clarify?<\/li>\n<li>\u274c or `:x:`: This has to change. It\u2019s possibly an error or strongly violates existing conventions.<\/li>\n<li>\ud83d\udd27 or `:wrench:`: This is a well-meant suggestion. Take it or leave it.<\/li>\n<li>\ud83d\ude43 or `:upside_down_face:`: This is a nitpick. Normally related to a small formatting or stylizing detail that shouldn\u2019t block moving forward.<\/li>\n<li>\ud83d\udcad or `:thought_balloon:`: I\u2019m just thinking out loud here. Something doesn\u2019t necessarily have to change, but I want to make sure to share my thoughts.<\/li>\n<li>\ud83e\udd21 or `:clown_face:`: This is a complaint about something with no obvious answer, not necessarily a problem originating from changes.<\/li>\n<\/ul>\n<p>The Emoji Code helps us separate well-meant suggestions, simple questions, and must-have requests that require code changes. It also adds an additional human component to the conversation, so we don\u2019t forget there\u2019s a human on the other side of the screen.<\/p>\n<p><em>*Just kidding; we haven\u2019t trademarked Emoji Code, so spread the word far and wide\u200aand make it your own!<\/em><\/p>\n<p><img decoding=\"async\" src=\"\" alt=\"An example code review, complemented by the Emoji Code.\" width=\"800\" class=\"aligncenter size-large wp-image-1086\" \/><\/p>\n<h2>Tips for Submitters<\/h2>\n<h3>Follow basic etiquette.<\/h3>\n<p>We\u2019ve established a few common ground rules, or \u201cMobile Center Code Review House Rules,\u201d\u200aas a baseline for all submitters.<\/p>\n<ul>\n<li><strong>Acknowledge prior feedback<\/strong>: It\u2019s frustrating to correct the same issue multiple times, so learn from earlier reviews and show reviewers that you respect and appreciate their advice.<\/li>\n<li><strong>Be clear<\/strong>: If you disagree with your reviewers\u2019 suggestions, make that clear. If you plan to implement your reviewers\u2019 suggestions, make that clear too.<\/li>\n<li><strong>Stay open-minded<\/strong>: Don\u2019t try to \u201cwin the argument,\u201d instead have a pragmatic conversation.<\/li>\n<li><strong>Limit code review discussion<\/strong>: If one or two comments back and forth doesn\u2019t resolve a problem, it won\u2019t be solved in code review. Instead, talk to the reviewer in person, on the phone, or via chat. Remember, it\u2019s okay to agree to disagree.<\/li>\n<li><strong>Embrace change<\/strong>: If you don\u2019t have a strong opinion, take the reviewer\u2019s suggestions.<\/li>\n<\/ul>\n<p>\u2026 and, for all-star code reviews: ensure all team members, including those spread across offices and time zones, get a chance to look at changes <em>before<\/em> you merge them*.<\/p>\n<p><em>*We\u2019ve become more conscious of features that allow \u201cauto-completion\u201d (automatically merging the changes whenever a CI build succeeds and at least one reviewer has approved the changes). Super useful, but, depending on the scenario, you may want to disable and give your teammates a chance to weigh in.<\/em><\/p>\n<h3>Remember that smaller =\u00a0better<\/h3>\n<p>Small pull requests are win-win-win. They\u2019re easier for reviewers to grasp right away and quicker to review since there\u2019s less code to review and comment on. For submitters, this fast turnaround makes it more likely that changes get merged faster, and any team member who depends on the code can start working on their next project.<\/p>\n<p>Bundling changes in business logic with refactorings of technically unrelated code in the same file might be tempting, but <strong><em>don\u2019t do it<\/em><\/strong>. This makes pull requests unnecessarily large and unwieldy, causing confusion for reviewers and delays in feedback.<\/p>\n<p>We love when people care for code quality and want to improve it, but create a separate pull request.<\/p>\n<h3>Respect the codebase and the core maintainers\u2019 decisions.<\/h3>\n<p>Anyone who\u2019s contributing to open source projects on GitHub, GitLab, or Bitbucket quickly learns that maintainers rarely accept code that doesn\u2019t fit existing styles and patterns. The same goes for different groups in the larger Mobile Center team; some members have different preferences than others. Therefore, it\u2019s important to respect the rules of the team that owns the codebase.<\/p>\n<p>Our guidelines:<\/p>\n<ul>\n<li><strong>Don\u2019t ask maintainers to accept contributions without proper review<\/strong>: \u201cLGTM\u201d (a.k.a. \u201cLooks Good To Me\u201d) is the easiest, least time-consuming reviewer response, but it\u2019s harmful to a codebase. If you know your reviewer only signed off because you applied heavy pressure (\u201cI\u2019m blocked by your review.\u201d), it doesn\u2019t help anyone.<\/li>\n<li><strong>Use existing indentation style<\/strong>: Be consistent with what\u2019s already there; nobody wants to have the age-old fight over tabs, spaces, 2 or 4, over and over again.<\/li>\n<li><strong>Add tests<\/strong>: Any serious project includes tests, and you\u2019re expected to add or adjust tests to cover your changes. If a project <em>doesn\u2019t<\/em> contain tests yet, they\u2019re always a good idea\u2014consider being a pioneer and authoring the first ones!<\/li>\n<li><strong>Recognize and incorporate architectures and coding patterns<\/strong>: Maintainers need to establish and enforce some level of consistency in how things are structured or abstracted so projects don\u2019t become a confusing mishmash. Identify these commonalities and adhere to them.<\/li>\n<\/ul>\n<h3>Be your #1\u00a0critic.<\/h3>\n<p><strong>Seriously\u2014review your own code<\/strong>! Create an actual pull request to see your code like your reviewer will. Similar to proof-reading paper text, you\u2019ll likely detect more errors.<\/p>\n<p>We\u2019ve uncovered lots of small issues across the Mobile Center team when we look at our changes as a whole, and we\u2019re able to fix them <em>before<\/em> anyone else spots them or wastes time catching a simple issue.<\/p>\n<h3>Respond to feedback\u00a0ASAP.<\/h3>\n<p>Review your feedback immediately, ask any necessary questions, and make required changes. If your pull request sits, your reviewer\u2019s feedback gets stale and it\u2019s likely that other code conflicts will render it un-mergeable. Check for reviewer comments or requests frequently (every one or two days), and act quickly.<\/p>\n<h2>Winning Code\u00a0Reviews<\/h2>\n<p>Code review is the epitome of software development: teamwork, quality control, problem solving, and continuous learning all in one. Regularly reading other people\u2019s code makes you a better programmer, because you\u2019re forced to understand other people\u2019s code and decision making and familiarize yourself with common patterns, coding styles, and business requirements, and <em>your<\/em> code benefits from it, too. On the other side, submitting code is humbling, forcing you to see what you\u2019ve built from an outside perspective and identify ways to make it <em>better.<\/em><\/p>\n<p>For us, the Emoji Code, keeping our comments human and friendly, and codifying our Code Review best practices make us better developers and a better team. The result: we stay focused, continuously committing and releasing code, features, and integrations to help <em>you<\/em> to deliver even better code and amazing apps.<\/p>\n<p><a target=\"_blank\" href=\"https:\/\/mobile.azure.com\/login\" rel=\"noopener noreferrer\">Log in or sign up for your free Mobile Center account<\/a>, connect your latest project, and share how <em>you<\/em> code review.<\/p>\n<p><em>Special thanks to <a target=\"_blank\" href=\"https:\/\/twitter.com\/pfleidi\" rel=\"noopener noreferrer\"><em>Sven<\/em><\/a> for writing large parts of our internal code review guidelines and generally pushing our engineering culture forward \ud83d\udcaa<\/em><\/p>\n","protected":false},"excerpt":{"rendered":"<p>Visual Studio Mobile Center is now Visual Studio App Center. Learn more here. &nbsp; The Mobile Center team is a diverse mix of engineers, product managers, and UI\/UX designers with varying professional backgrounds, experience, and countries of origin. Our team has people from over 10 countries working across 8 time zones. Some are fresh out [&hellip;]<\/p>\n","protected":false},"author":35,"featured_media":38034,"comment_status":"open","ping_status":"closed","sticky":false,"template":"","format":"standard","meta":{"_acf_changed":false,"footnotes":""},"categories":[16],"tags":[],"class_list":["post-435","post","type-post","status-publish","format-standard","has-post-thumbnail","hentry","category-mobiledev"],"acf":[],"blog_post_summary":"<p>Visual Studio Mobile Center is now Visual Studio App Center. Learn more here. &nbsp; The Mobile Center team is a diverse mix of engineers, product managers, and UI\/UX designers with varying professional backgrounds, experience, and countries of origin. Our team has people from over 10 countries working across 8 time zones. Some are fresh out [&hellip;]<\/p>\n","_links":{"self":[{"href":"https:\/\/devblogs.microsoft.com\/appcenter\/wp-json\/wp\/v2\/posts\/435","targetHints":{"allow":["GET"]}}],"collection":[{"href":"https:\/\/devblogs.microsoft.com\/appcenter\/wp-json\/wp\/v2\/posts"}],"about":[{"href":"https:\/\/devblogs.microsoft.com\/appcenter\/wp-json\/wp\/v2\/types\/post"}],"author":[{"embeddable":true,"href":"https:\/\/devblogs.microsoft.com\/appcenter\/wp-json\/wp\/v2\/users\/35"}],"replies":[{"embeddable":true,"href":"https:\/\/devblogs.microsoft.com\/appcenter\/wp-json\/wp\/v2\/comments?post=435"}],"version-history":[{"count":0,"href":"https:\/\/devblogs.microsoft.com\/appcenter\/wp-json\/wp\/v2\/posts\/435\/revisions"}],"wp:featuredmedia":[{"embeddable":true,"href":"https:\/\/devblogs.microsoft.com\/appcenter\/wp-json\/wp\/v2\/media\/38034"}],"wp:attachment":[{"href":"https:\/\/devblogs.microsoft.com\/appcenter\/wp-json\/wp\/v2\/media?parent=435"}],"wp:term":[{"taxonomy":"category","embeddable":true,"href":"https:\/\/devblogs.microsoft.com\/appcenter\/wp-json\/wp\/v2\/categories?post=435"},{"taxonomy":"post_tag","embeddable":true,"href":"https:\/\/devblogs.microsoft.com\/appcenter\/wp-json\/wp\/v2\/tags?post=435"}],"curies":[{"name":"wp","href":"https:\/\/api.w.org\/{rel}","templated":true}]}}