larlet-fr-david-cache/cache/5544b85aba890be84b09b4c637c78bac/index.md

title: Giving better code reviews
url: https://medium.com/@mrjoelkemp/giving-better-code-reviews-16109e0fdd36
hash_url: 5544b85aba890be84b09b4c637c78bac

<p name="48ef" id="48ef" class="graf--p graf-after--h3">I take code reviews very seriously. It’s one of the crucial processes for ensuring the spread of knowledge transfer and best practices throughout a development team.</p><p name="e1f8" id="e1f8" class="graf--p graf-after--p">As important as code reviews are, we typically leave it up to the reviewer to improve his/her process. This results in an inconsistent review quality across a team. The poor reviewers become known and folks looking for an easy approval know who to go to.</p><p name="d923" id="d923" class="graf--p graf-after--p"><strong class="markup--strong markup--p-strong">Your team is only as good as your weakest reviewer.</strong></p><p name="0853" id="0853" class="graf--p graf-after--p">There are at least two phases of reviewing code. Most stop at the first (least valuable) phase.</p><h4 name="b0da" id="b0da" class="graf--h4 graf-after--p">Phase 1: The light pass</h4><p name="6270" id="6270" class="graf--p graf-after--h4">In this first pass of a code diff, we typically look for the following:</p><ul class="postList"><li name="c47b" id="c47b" class="graf--li graf-after--p">Obvious mistakes: typos, poor variable names, unclear function names, large function definitions.</li><li name="9950" id="9950" class="graf--li graf-after--li">At least one test. This assumes mandatory tests are part of the dev culture.</li><li name="ea6a" id="ea6a" class="graf--li graf-after--li">Commented out code that should be deleted.</li><li name="d48b" id="d48b" class="graf--li graf-after--li">Violations of our styleguide that linters don’t catch yet.</li></ul><p name="4108" id="4108" class="graf--p graf-after--li">If the pull request author proofread their diff (which they should be doing) before pinging for a review, then there’s a good chance that there are only a few violations of the above points. Many reviewers, at this point, will just go ahead and give the “looks good to me” (LGTM). Also at this phase, sloppy or lazy reviewers will do a “terms of service” review: scroll, scroll, scroll, accept. We can do better.</p><h4 name="ace7" id="ace7" class="graf--h4 graf-after--p">Phase 2: The contextual pass</h4><p name="d77d" id="d77d" class="graf--p graf-after--h4">This deeper pass of the diff potentially takes a lot of time. Many reviewers are unsure of how much time to spend on a review, so they’ll punt on digging deep into a diff and trust the author.</p><p name="36f9" id="36f9" class="graf--p graf-after--p">Trust no one. Question everything (kindly). Assume that the author has made mistakes that you need to catch. Save your users from those bugs.</p><p name="9b2a" id="9b2a" class="graf--p graf-after--p">As an aside, I find that this trust happens a ton with less experienced developers. They believe that authors with more experience <em class="markup--em markup--p-em">must</em> know what they’re doing, so there’s no need to question the code. Make sure that you address and correct this assumption. This deprives less experienced devs with the context that they desperately need.</p><p name="773f" id="773f" class="graf--p graf-after--p">In this contextual phase, we’re looking for the following:</p><ul class="postList"><li name="464f" id="464f" class="graf--li graf-after--p">More idiomatic ways of using our frameworks/libraries.</li><li name="0516" id="0516" class="graf--li graf-after--li">Adequate test coverage for changed lines or critical code paths.</li><li name="073f" id="073f" class="graf--li graf-after--li">That the tests (non-trivially) pass, assert a valuable behavior, and do not (if you can help it) test implementation details.</li><li name="eeb9" id="eeb9" class="graf--li graf-after--li">Ways of strengthening tests</li><li name="35fd" id="35fd" class="graf--li graf-after--li">Alternative implementations or <a href="https://sourcemaking.com/refactoring" data-href="https://sourcemaking.com/refactoring" class="markup--anchor markup--li-anchor" rel="nofollow">refactors</a> that increase readability/understandability and/or maintainability.</li><li name="c21b" id="c21b" class="graf--li graf-after--li">Unhandled edge-cases based on data-type/data-existence assumptions in a function.</li><li name="9ee2" id="9ee2" class="graf--li graf-after--li">That you can actually understand what the code is doing. I used to think it was my fault for not being able to understand part of a diff. It’s not. Hard to reason about code should be corrected. If <em class="markup--em markup--li-em">you’re</em> struggling to understand, folks with even less experience have no hope.</li><li name="4c1a" id="4c1a" class="graf--li graf-after--li">Can you gather context for that diff? i.e., if you’re totally unfamiliar with that part of the codebase, could you roughly tell what that part of the system does and how the diff affects it?</li><li name="e3ca" id="e3ca" class="graf--li graf-after--li">Logical omissions or errors</li><li name="817f" id="817f" class="graf--li graf-after--li">If you know the feature requirements, look for spec omissions. “Shouldn’t this component also do X?”</li></ul><p name="26bc" id="26bc" class="graf--p graf-after--li">Feel free to ask questions on particular lines. It’s okay for you to want to know what’s going on; it’s not a burden on the author. It’s crucial that this context is spread across many team members.</p><p name="0a72" id="0a72" class="graf--p graf-after--p">Also be aware of how you ask questions. Suggest approaches or provoke exploration. Try not to give solutions.</p><h4 name="b245" id="b245" class="graf--h4 graf-after--p">But this diff will take forever to review</h4><p name="250e" id="250e" class="graf--p graf-after--h4">Giving a valuable code review requires that pull requests are small (&lt;200 lines as a rough gauge). It’s unfair to give a reviewer a diff consisting of hundreds or thousands of lines of code changes. They’ll end up spending anywhere up to 30 minutes reviewing that diff and then have to do re-reviews when you make suggested changes. There’s definitely an inverse correlation between lines of code and review quality.</p><p name="ae6a" id="ae6a" class="graf--p graf-after--p">Sometimes, folks from other feature teams will send you a diff to review, but you might not have any real context as to what they’re working on. Naturally, it would take you a long time to gather enough context to give an effective review. In those cases, I tell the author that I can’t really give a quality review on that diff and that they should look for another reviewer. If no one else is available, I’d sit with the author and have them talk me through the diff — allowing me to ask questions at each step. It’s a time sink for both of us, but we, the code, and our users will end up better-off for the detailed analysis.</p><p name="90d0" id="90d0" class="graf--p graf-after--p">We have to be pragmatic though. Issues with a hotfix can be addressed in a follow-up diff. Have we hit the point of diminishing returns with our comments? Are we spinning our wheels with subjective nitpicks?</p><p name="5703" id="5703" class="graf--p graf-after--p">If you’d like to save some time, consider labeling “blocking” and “non-blocking” review comments. If a reviewer has the time and agrees with the benefit in the non-blocking comment, they can address it.</p><h4 name="1d8f" id="1d8f" class="graf--h4 graf-after--p">Can we do even better?</h4><p name="fd02" id="fd02" class="graf--p graf-after--h4">I firmly believe that reviewing code is as much of a craft as writing code. As reviewers, it’s our mission to improve on a number of fronts: our ability to understand code; our process, awareness, and criteria for analysis; and our teammates’ ability to write correct diffs.</p><p name="4c19" id="4c19" class="graf--p graf-after--p graf--last">I’d love to hear your thoughts/improvements/additions on my guidelines for reviewing diffs. Find me on twitter: <a href="https://twitter.com/mrjoelkemp" data-href="https://twitter.com/mrjoelkemp" class="markup--anchor markup--p-anchor" rel="nofollow">mrjoelkemp</a>.</p>