-
Notifications
You must be signed in to change notification settings - Fork 0
/
how-to-do-a-code-review.html
18 lines (17 loc) · 15.2 KB
/
how-to-do-a-code-review.html
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
<!DOCTYPE html><html lang="de-ch"><head><meta charset="utf-8"><meta http-equiv="X-UA-Compatible" content="IE=edge"><meta name="viewport" content="width=device-width,initial-scale=1"><title>How to do a code review - Finecloud</title><meta name="description" content="This Blog post is my personal summary of Googles code review process Make sure to review every line of code you’ve been asked to review, look at the context, make sure you’re improving code health, and compliment developers on good things that they do. Look at…"><meta name="generator" content="Publii Open-Source CMS for Static Site"><link rel="stylesheet" href="https://www.finecloud.ch/media/plugins/syntaxHighlighter/prism-black.css"><link rel="canonical" href="https://www.finecloud.ch/how-to-do-a-code-review.html"><link rel="alternate" type="application/atom+xml" href="https://www.finecloud.ch/feed.xml"><link rel="alternate" type="application/json" href="https://www.finecloud.ch/feed.json"><meta property="og:title" content="How to do a code review"><meta property="og:site_name" content="Finecloud"><meta property="og:description" content="This Blog post is my personal summary of Googles code review process Make sure to review every line of code you’ve been asked to review, look at the context, make sure you’re improving code health, and compliment developers on good things that they do. Look at…"><meta property="og:url" content="https://www.finecloud.ch/how-to-do-a-code-review.html"><meta property="og:type" content="article"><link rel="shortcut icon" href="https://www.finecloud.ch/media/website/finecloud.png" type="image/png"><link rel="stylesheet" href="https://www.finecloud.ch/assets/css/style.css?v=39da73365516a098a9b73b721fc970e2"><script type="application/ld+json">{"@context":"http://schema.org","@type":"Article","mainEntityOfPage":{"@type":"WebPage","@id":"https://www.finecloud.ch/how-to-do-a-code-review.html"},"headline":"How to do a code review","datePublished":"2024-03-18T08:44","dateModified":"2024-03-18T17:33","description":"This Blog post is my personal summary of Googles code review process Make sure to review every line of code you’ve been asked to review, look at the context, make sure you’re improving code health, and compliment developers on good things that they do. Look at…","author":{"@type":"Person","name":"Finecloud","url":"https://www.finecloud.ch/authors/finecloud/"},"publisher":{"@type":"Organization","name":"Finecloud"}}</script><meta name="google-site-verification" content="seFY9U12uiEq5U3_MyZiX6XWzk0AVFl9zITr2ZKsytY"></head><body><div class="site-container"><header class="top" id="js-header"><a class="logo" href="https://www.finecloud.ch/">Finecloud</a><nav class="navbar js-navbar"><button class="navbar__toggle js-toggle" aria-label="Menu" aria-haspopup="true" aria-expanded="false"><span class="navbar__toggle-box"><span class="navbar__toggle-inner">Menu</span></span></button><ul class="navbar__menu"><li><a href="https://www.finecloud.ch/" target="_self">Blog</a></li><li><a href="https://www.finecloud.ch/tags/" target="_self">Tags</a></li></ul></nav><div class="search"><div class="search__overlay js-search-overlay"><div class="search__overlay-inner"><form action="https://www.finecloud.ch/search.html" class="search__form"><input class="search__input js-search-input" type="search" name="q" placeholder="search..." aria-label="search..." autofocus="autofocus"></form><button class="search__close js-search-close" aria-label="Close">Close</button></div></div><button class="search__btn js-search-btn" aria-label="Search"><svg role="presentation" focusable="false"><use xlink:href="https://www.finecloud.ch/assets/svg/svg-map.svg#search"/></svg></button></div></header><main><article class="post"><div class="hero"><figure class="hero__image hero__image--overlay"><img src="https://www.finecloud.ch/media/website/download.jpg" srcset="https://www.finecloud.ch/media/website/responsive/download-xs.jpg 300w, https://www.finecloud.ch/media/website/responsive/download-sm.jpg 480w, https://www.finecloud.ch/media/website/responsive/download-md.jpg 768w, https://www.finecloud.ch/media/website/responsive/download-lg.jpg 1024w, https://www.finecloud.ch/media/website/responsive/download-xl.jpg 1360w, https://www.finecloud.ch/media/website/responsive/download-2xl.jpg 1600w" sizes="100vw" loading="eager" alt=""></figure><header class="hero__content"><div class="wrapper"><div class="post__meta"><time datetime="2024-03-18T08:44">März 18, 2024</time></div><h1>How to do a code review</h1></div></header></div><div class="wrapper post__entry"><p>This Blog post is my personal summary of <a href="https://google.github.io/eng-practices/review/" target="_blank" rel="nofollow noopener">Googles code review process</a></p><p></p><div class="post__toc"><h3>Table of contents</h3><ul><li><a href="#summary-of-what-you-should-look-at">Summary of what you should look at</a></li><li><a href="#the-standard-of-code-review">The Standard of Code Review</a></li><li><a href="#navigate-a-pull-request-pr-in-review">Navigate a Pull-Request (PR) in review</a><ul><li><a href="#1-take-a-broad-view-of-the-change">1. Take a broad view of the change</a></li><li><a href="#2-examine-the-main-parts-of-the-pr">2. Examine the main parts of the PR</a></li><li><a href="#3andnbsplook-through-the-rest-of-the-pr-in-an-appropriate-sequence">3. Look through the rest of the PR in an appropriate sequence</a></li></ul></li><li><a href="#speed-of-code-reviews">Speed of Code Reviews</a><ul><li><a href="#approve-with-comments">Approve with Comments</a></li></ul></li><li><a href="#how-to-write-comments">How to write comments</a></li></ul></div><h2 id="summary-of-what-you-should-look-at">Summary of what you should look at</h2><ul><li><strong>Design</strong>: Is the code well-designed and appropriate for your system?<br></li><li><strong>Functionality</strong>: Does the code behave as the author likely intended? Is the way the code behaves good for its users?</li><li><strong>Complexity</strong>: Could the code be made simpler? Would another developer be able to easily understand and use this code when they come across it in the future?</li><li><strong>Tests</strong>: Does the code have correct and well-designed automated tests?<br></li><li><strong>Naming</strong>: Did the developer choose clear names for variables, classes, methods, etc.?</li><li><strong>Comments</strong>: Are the comments clear and useful?</li><li><strong>Style</strong>: Does the code follow our style guides?</li><li><strong>Documentation</strong>: Did the developer also update relevant documentation?<br></li></ul><p>Make sure to review <strong>every line</strong> of code you’ve been asked to review, look at the <strong>context</strong>, make sure you’re <strong>improving code health</strong>, and compliment developers on <strong>g</strong><strong>ood things</strong> that they do.</p><h2 id="the-standard-of-code-review">The Standard of Code Review</h2><ul><li>You need to balance the tradeoff between making progress, by letting a change go into your code base and not decreasing overall code health and quality. </li><li><span style="color: var(--text-primary-color); font-family: var(--editor-font-family); font-size: 1em; font-weight: var(--font-weight-normal);">does the Pull-Request improve the maintainability, readability, and understandability?</span><br></li><li><strong>In general you should try to approve a Pull-Request (PR) once it is in a state where it definitely improves the overall code health, even if the PR isn’t perfect.</strong></li><li>Reviewers should not require the author to polish every tiny piece of a CL before granting approval. Rather, the reviewer should balance out the need to make forward progress compared to the importance of the changes they are suggesting. </li><li>Instead of seeking perfection, what a reviewer should seek is continuous improvement.</li><li>If you add a comment on a PR to mention something could be better, but it’s optional and not very important, prefix it with something like “Nit: “ to let the author know that it’s just a point of polish that they could choose to ignore.</li></ul><h2 id="navigate-a-pull-request-pr-in-review">Navigate a Pull-Request (PR) in review</h2><h3 id="1-take-a-broad-view-of-the-change">1. Take a broad view of the change</h3><p>Look at the PR description and what the PR does in general. Does this change even make sense? If this change shouldn’t have happened in the first place, please respond immediately with an explanation of why the change should not be happening. When you reject a change like this, it’s also a good idea to suggest to the developer what they should have done instead.</p><h3 id="2-examine-the-main-parts-of-the-pr">2. Examine the main parts of the PR</h3><p>Find the file or files that are the “main” part of this PR. Often, there is one file that has the largest number of logical changes, and it’s the major piece of the PR. Look at these major parts first. This helps give context to all of the smaller parts of the PR, and generally accelerates doing the code review. </p><h3 id="3andnbsplook-through-the-rest-of-the-pr-in-an-appropriate-sequence">3. Look through the rest of the PR in an appropriate sequence</h3><p>Once you’ve confirmed there are no major design problems with the CL as a whole, try to figure out a logical sequence to look through the files while also making sure you don’t miss reviewing any file.</p><h2 id="speed-of-code-reviews">Speed of Code Reviews</h2><p>Why is it so important that you send comments out immediately, especially if you see major design problems within a PR?</p><ul><li>Developers often open a PR and then immediately start new work based on that PR (e.g. feature branch) while they wait for review. If there are major design problems in the PR you’re reviewing, they’re also going to have to re-work their later PR. You want to catch them before they’ve done too much extra work on top of the problematic design.</li><li>Major design changes take longer to do than small changes. Developers nearly all have deadlines; in order to make those deadlines and still have quality code in the codebase, the developer needs to start on any major re-work of the PR as soon as possible.<br></li></ul><p>When code reviews are slow, several things happen:</p><ul><li>The velocity of the team as a whole is decreased.</li><li>Developers start to protest the code review process. Most complaints about the code review process are actually resolved by making the process faster.</li><li>Code health can be impacted.<br></li></ul><p>How fast should a code review be? </p><ul><li>If you are not in the middle of a focused task, <strong>you should do a code review shortly after it comes in</strong>.</li><li><strong>One business day is the maximum time it should take to respond</strong> to a code review request (i.e., first thing the next morning).</li></ul><p><strong>If you are in the middle of a focused task, such as writing code, don’t interrupt yourself to do a code review</strong>. Research has shown that it can take a long time for a developer to get back into a smooth flow of development after being interrupted. So interrupting yourself while coding is actually more expensive to the team than making another developer wait a bit for a code review.</p><h3 id="approve-with-comments">Approve with Comments</h3><p>In order to speed up code reviews, there are certain situations in which a reviewer should give the Approval even though they are also leaving unresolved comments on the PR. This is done when either:</p><ul><li>The reviewer is confident that the developer will appropriately address all the reviewer’s remaining comments.</li><li>The remaining changes are minor and don’t have to be done by the developer.</li></ul><h2 id="how-to-write-comments">How to write comments</h2><ul><li>Be kind.</li><li>Explain your reasoning (why?).</li><li>Balance giving explicit directions with just pointing out problems and letting the developer decide -> In general it is the developer’s responsibility to fix a PR, not the reviewer’s.</li><li>Encourage developers to simplify code or add code comments instead of just explaining the complexity to you.<br></li></ul></div><footer class="wrapper post__footer"><p class="post__last-updated">This article was updated on März 18, 2024</p><ul class="post__tag"><li><a href="https://www.finecloud.ch/tags/devops/">devops</a></li><li><a href="https://www.finecloud.ch/tags/softwareentwicklung/">software development</a></li></ul><div class="post__share"></div></footer></article><nav class="post__nav"><div class="post__nav-inner"><div class="post__nav-prev"><svg width="1.041em" height="0.416em" aria-hidden="true"><use xlink:href="https://www.finecloud.ch/assets/svg/svg-map.svg#arrow-prev"/></svg> <a href="https://www.finecloud.ch/building-a-graphql-service.html" class="post__nav-link" rel="prev"><span>Previous</span> Building a GraphQL service</a></div><div class="post__nav-next"><a href="https://www.finecloud.ch/awk-and-sed.html" class="post__nav-link" rel="next"><span>Next</span> Awk and Sed </a><svg width="1.041em" height="0.416em" aria-hidden="true"><use xlink:href="https://www.finecloud.ch/assets/svg/svg-map.svg#arrow-next"/></svg></div></div></nav><div class="post__related related"><div class="wrapper"><h2 class="h5 related__title">You should also read:</h2><article class="related__item"><div class="feed__meta"><time datetime="2024-04-05T21:02" class="feed__date">April 5, 2024</time></div><h3 class="h1"><a href="https://www.finecloud.ch/github-codespace.html">GitHub Codespace</a></h3></article><article class="related__item"><div class="feed__meta"><time datetime="2023-02-09T16:55" class="feed__date">Februar 9, 2023</time></div><h3 class="h1"><a href="https://www.finecloud.ch/cleancode.html">CleanCode</a></h3></article><article class="related__item"><div class="feed__meta"><time datetime="2022-06-27T06:29" class="feed__date">Juni 27, 2022</time></div><h3 class="h1"><a href="https://www.finecloud.ch/http-response-status-codes.html">HTTP Response Status Codes</a></h3></article></div></div></main><footer class="footer"><div class="footer__copyright"><p>Powered by Publii</p></div><button onclick="backToTopFunction()" id="backToTop" class="footer__bttop" aria-label="Back to top" title="Back to top"><svg><use xlink:href="https://www.finecloud.ch/assets/svg/svg-map.svg#toparrow"/></svg></button></footer></div><script>window.publiiThemeMenuConfig = {
mobileMenuMode: 'sidebar',
animationSpeed: 300,
submenuWidth: 'auto',
doubleClickTime: 500,
mobileMenuExpandableSubmenus: true,
relatedContainerForOverlayMenuSelector: '.top',
};</script><script defer="defer" src="https://www.finecloud.ch/assets/js/scripts.min.js?v=6ca8b60e6534a3888de1205e82df8528"></script><script>var images = document.querySelectorAll('img[loading]');
for (var i = 0; i < images.length; i++) {
if (images[i].complete) {
images[i].classList.add('is-loaded');
} else {
images[i].addEventListener('load', function () {
this.classList.add('is-loaded');
}, false);
}
}</script><script defer="defer" src="https://www.finecloud.ch/media/plugins/syntaxHighlighter/prism.js"></script></body></html>