davidbgk
/
larlet-fr-david-cache
Слідкувати 1
В обрані 0
Форк 0
Код Релізи 0 Активність
A place to cache linked articles (think custom and personal wayback machine)
Ви не можете вибрати більше 25 тем Теми мають розпочинатися з літери або цифри, можуть містити дефіси (-) і не повинні перевищувати 35 символів.
396 Коміти
1 Гілка
Дерево: f82e56a712
Гілки Теги
${ item.name }
Створити гілку ${ searchTerm }
з 'f82e56a712'
${ noResults }
larlet-fr-david-cache/cache/5544b85aba890be84b09b4c637c.../index.html

index.html 24KB
Неформатований Blame Історія

123456789101112131415161718192021222324252627282930313233343536373839404142434445464748495051525354555657585960616263646566676869707172737475767778798081828384858687888990919293949596979899100101102103104105106107108109110111112113114115116117118119120121122123124125126127128129130131132133134135136137138139140141142143144145146147148149150151152153154155156157158159160161162163164165166167168169170171172173174175176177178179180181182183184185186187188189190191192193194195196197198199200201202203204205206207208209210211212213214215216217218219220221222223224225226227228229230231232233234235236237238239240241242243244245246247248249250251252253254255256257258259260261262263264265266267268269270271272273274275276277278279280281282283284285286287288289290291292293294295296297298299300301302303304305306307308309310311312313314315316317318319320321322323324325326327328329330331332333334335336337338339340341342343344345346347348349350351352353354355356357358359360361362363364365366367368369370371372373374375376377378379380381382383384385386387388389390391392393394395396397398399400401402403404405406407408409410411412413414415416417418419420421422423424425426427428429430431432433434435436437438439440441442443444445446447448449450451452453454455456457458459460461462463464465466467468469470471472473474475476477478479480481482483484485486487488489490491492493494495496497498499500501502503504505506507508509510511512513514515516517518519520521522523524525526527528529530531532533534535536537538539540541542543544545546547548549550551552553554555556557558559560561562563564565566567568569570571572573 
  1. <!doctype html><!-- This is a valid HTML5 document. -->

  2. <!-- Screen readers, SEO, extensions and so on. -->

  3. <html lang=fr>

  4. <!-- Has to be within the first 1024 bytes, hence before the <title>

  5.   See: https://www.w3.org/TR/2012/CR-html5-20121217/document-metadata.html#charset -->

  6. <meta charset=utf-8>

  7. <!-- Why no `X-UA-Compatible` meta: https://stackoverflow.com/a/6771584 -->

  8. <!-- The viewport meta is quite crowded and we are responsible for that.

  9.   See: https://codepen.io/tigt/post/meta-viewport-for-2015 -->

  10. <meta name=viewport content="width=device-width,minimum-scale=1,initial-scale=1,shrink-to-fit=no">

  11. <!-- Required to make a valid HTML5 document. -->

  12. <title>Giving better code reviews (archive) — David Larlet</title>

  13. <!-- Generated from https://realfavicongenerator.net/ such a mess. -->

  14. <link rel="apple-touch-icon" sizes="180x180" href="/static/david/icons/apple-touch-icon.png">

  15. <link rel="icon" type="image/png" sizes="32x32" href="/static/david/icons/favicon-32x32.png">

  16. <link rel="icon" type="image/png" sizes="16x16" href="/static/david/icons/favicon-16x16.png">

  17. <link rel="manifest" href="/manifest.json">

  18. <link rel="mask-icon" href="/static/david/icons/safari-pinned-tab.svg" color="#5bbad5">

  19. <link rel="shortcut icon" href="/static/david/icons/favicon.ico">

  20. <meta name="apple-mobile-web-app-title" content="David Larlet">

  21. <meta name="application-name" content="David Larlet">

  22. <meta name="msapplication-TileColor" content="#da532c">

  23. <meta name="msapplication-config" content="/static/david/icons/browserconfig.xml">

  24. <meta name="theme-color" content="#f0f0ea">

  25. <!-- That good ol' feed, subscribe :p. -->

  26. <link rel=alternate type="application/atom+xml" title=Feed href="/david/log/">

  27. 

  28.   <meta name="robots" content="noindex, nofollow">

  29.   <meta content="origin-when-cross-origin" name="referrer">

  30.   <!-- Canonical URL for SEO purposes -->

  31.   <link rel="canonical" href="https://medium.com/@mrjoelkemp/giving-better-code-reviews-16109e0fdd36">

  32. 

  33. <style>

  34. /* http://meyerweb.com/eric/tools/css/reset/ */

  35. html, body, div, span,

  36. h1, h2, h3, h4, h5, h6, p, blockquote, pre,

  37. a, abbr, address, big, cite, code,

  38. del, dfn, em, img, ins,

  39. small, strike, strong, tt, var,

  40. dl, dt, dd, ol, ul, li,

  41. fieldset, form, label, legend,

  42. table, caption, tbody, tfoot, thead, tr, th, td,

  43. article, aside, canvas, details, embed,

  44. figure, figcaption, footer, header, hgroup,

  45. menu, nav, output, ruby, section, summary,

  46. time, mark, audio, video {

  47.   margin: 0;

  48.   padding: 0;

  49.   border: 0;

  50.   font-size: 100%;

  51.   font: inherit;

  52.   vertical-align: baseline;

  53. }

  54. /* HTML5 display-role reset for older browsers */

  55. article, aside, details, figcaption, figure,

  56. footer, header, hgroup, menu, nav, section { display: block; }

  57. body { line-height: 1; }

  58. blockquote, q { quotes: none; }

  59. blockquote:before, blockquote:after,

  60. q:before, q:after {

  61.   content: '';

  62.   content: none;

  63. }

  64. table {

  65.   border-collapse: collapse;

  66.   border-spacing: 0;

  67. }

  68. 

  69. /* http://practicaltypography.com/equity.html */

  70. /* https://calendar.perfplanet.com/2016/no-font-face-bulletproof-syntax/ */

  71. /* https://www.filamentgroup.com/lab/js-web-fonts.html */

  72. @font-face {

  73.   font-family: 'EquityTextB';

  74.   src: url('/static/david/css/fonts/Equity-Text-B-Regular-webfont.woff2') format('woff2'),

  75.        url('/static/david/css/fonts/Equity-Text-B-Regular-webfont.woff') format('woff');

  76.   font-weight: 300;

  77.   font-style: normal;

  78.   font-display: swap;

  79. }

  80. @font-face {

  81.   font-family: 'EquityTextB';

  82.   src: url('/static/david/css/fonts/Equity-Text-B-Italic-webfont.woff2') format('woff2'),

  83.        url('/static/david/css/fonts/Equity-Text-B-Italic-webfont.woff') format('woff');

  84.   font-weight: 300;

  85.   font-style: italic;

  86.   font-display: swap;

  87. }

  88. @font-face {

  89.   font-family: 'EquityTextB';

  90.   src: url('/static/david/css/fonts/Equity-Text-B-Bold-webfont.woff2') format('woff2'),

  91.        url('/static/david/css/fonts/Equity-Text-B-Bold-webfont.woff') format('woff');

  92.   font-weight: 700;

  93.   font-style: normal;

  94.   font-display: swap;

  95. }

  96. 

  97. @font-face {

  98.   font-family: 'ConcourseT3';

  99.   src: url('/static/david/css/fonts/concourse_t3_regular-webfont-20190806.woff2') format('woff2'),

  100.        url('/static/david/css/fonts/concourse_t3_regular-webfont-20190806.woff') format('woff');

  101.   font-weight: 300;

  102.   font-style: normal;

  103.   font-display: swap;

  104. }

  105. 

  106. 

  107. /* http://practice.typekit.com/lesson/caring-about-opentype-features/ */

  108. body {

  109.   /* http://www.cssfontstack.com/ Palatino 99% Win 86% Mac */

  110.   font-family: "EquityTextB", Palatino, serif;

  111.   background-color: #f0f0ea;

  112.   color: #07486c;

  113.   font-kerning: normal;

  114.   -moz-osx-font-smoothing: grayscale;

  115.   -webkit-font-smoothing: subpixel-antialiased;

  116.   text-rendering: optimizeLegibility;

  117.   font-variant-ligatures: common-ligatures contextual;

  118.   font-feature-settings: "kern", "liga", "clig", "calt";

  119. }

  120. pre, code, kbd, samp, var, tt {

  121.   font-family: 'TriplicateT4c', monospace;

  122. }

  123. em {

  124.   font-style: italic;

  125.   color: #323a45;

  126. }

  127. strong {

  128.   font-weight: bold;

  129.   color: black;

  130. }

  131. nav {

  132.   background-color: #323a45;

  133.   color: #f0f0ea;

  134.   display: flex;

  135.   justify-content: space-around;

  136.   padding: 1rem .5rem;

  137. }

  138.   nav:last-child {

  139.     border-bottom: 1vh solid #2d7474;

  140.   }

  141.   nav a {

  142.     color: #f0f0ea;

  143.   }

  144.   nav abbr {

  145.     border-bottom: 1px dotted white;

  146.   }

  147. 

  148. h1 {

  149.   border-top: 1vh solid #2d7474;

  150.   border-bottom: .2vh dotted #2d7474;

  151.   background-color: #e3e1e1;

  152.   color: #323a45;

  153.   text-align: center;

  154.   padding: 5rem 0 4rem 0;

  155.   width: 100%;

  156.   font-family: 'ConcourseT3';

  157.   display: flex;

  158.   flex-direction: column;

  159. }

  160.   h1.single {

  161.     padding-bottom: 10rem;

  162.   }

  163.   h1 span {

  164.     position: absolute;

  165.     top: 1vh;

  166.     left: 20%;

  167.     line-height: 0;

  168.   }

  169.     h1 span a {

  170.       line-height: 1.7;

  171.       padding: 1rem 1.2rem .6rem 1.2rem;

  172.       border-radius: 0 0 6% 6%;

  173.       background: #2d7474;

  174.       font-size: 1.3rem;

  175.       color: white;

  176.       text-decoration: none;

  177.     }

  178. h2 {

  179.   margin: 4rem 0 1rem;

  180.   border-top: .2vh solid #2d7474;

  181.   padding-top: 1vh;

  182. }

  183. h3 {

  184.   text-align: center;

  185.   margin: 3rem 0 .75em;

  186. }

  187. hr {

  188.   height: .4rem;

  189.   width: .4rem;

  190.   border-radius: .4rem;

  191.   background: #07486c;

  192.   margin: 2.5rem auto;

  193. }

  194. time {

  195.   display: bloc;

  196.   margin-left: 0 !important;

  197. }

  198. ul, ol {

  199.   margin: 2rem;

  200. }

  201. ul {

  202.   list-style-type: square;

  203. }

  204. a {

  205.   text-decoration-skip-ink: auto;

  206.   text-decoration-thickness: 0.05em;

  207.   text-underline-offset: 0.09em;

  208. }

  209. article {

  210.   max-width: 50rem;

  211.   display: flex;

  212.   flex-direction: column;

  213.   margin: 2rem auto;

  214. }

  215.   article.single {

  216.     border-top: .2vh dotted #2d7474;

  217.     margin: -6rem auto 1rem auto;

  218.     background: #f0f0ea;

  219.     padding: 2rem;

  220.   }

  221.   article p:last-child {

  222.     margin-bottom: 1rem;

  223.   }

  224. p {

  225.   padding: 0 .5rem;

  226.   margin-left: 3rem;

  227. }

  228.   p + p,

  229.   figure + p {

  230.     margin-top: 2rem;

  231.   }

  232. 

  233. blockquote {

  234.   background-color: #e3e1e1;

  235.   border-left: .5vw solid #2d7474;

  236.   display: flex;

  237.   flex-direction: column;

  238.   align-items: center;

  239.   padding: 1rem;

  240.   margin: 1.5rem;

  241. }

  242.   blockquote cite {

  243.     font-style: italic;

  244.   }

  245.   blockquote p {

  246.     margin-left: 0;

  247.   }

  248. 

  249. figure {

  250.   border-top: .2vh solid #2d7474;

  251.   background-color: #e3e1e1;

  252.   text-align: center;

  253.   padding: 1.5rem 0;

  254.   margin: 1rem 0 0;

  255.   font-size: 1.5rem;

  256.   width: 100%;

  257. }

  258.   figure img {

  259.     max-width: 250px;

  260.     max-height: 250px;

  261.     border: .5vw solid #323a45;

  262.     padding: 1px;

  263.   }

  264. figcaption {

  265.   padding: 1rem;

  266.   line-height: 1.4;

  267. }

  268. aside {

  269.   display: flex;

  270.   flex-direction: column;

  271.   background-color: #e3e1e1;

  272.   padding: 1rem 0;

  273.   border-bottom: .2vh solid #07486c;

  274. }

  275.   aside p {

  276.     max-width: 50rem;

  277.     margin: 0 auto;

  278.   }

  279. 

  280. /* https://fvsch.com/code/css-locks/ */

  281. p, li, pre, code, kbd, samp, var, tt, time, details, figcaption {

  282.   font-size: 1rem;

  283.   line-height: calc( 1.5em + 0.2 * 1rem );

  284. }

  285. h1 {

  286.   font-size: 1.9rem;

  287.   line-height: calc( 1.2em + 0.2 * 1rem );

  288. }

  289. h2 {

  290.   font-size: 1.6rem;

  291.   line-height: calc( 1.3em + 0.2 * 1rem );

  292. }

  293. h3 {

  294.   font-size: 1.35rem;

  295.   line-height: calc( 1.4em + 0.2 * 1rem );

  296. }

  297. @media (min-width: 20em) {

  298.   /* The (100vw - 20rem) / (50 - 20) part

  299.      resolves to 0-1rem, depending on the

  300.      viewport width (between 20em and 50em). */

  301.   p, li, pre, code, kbd, samp, var, tt, time, details, figcaption {

  302.     font-size: calc( 1rem + .6 * (100vw - 20rem) / (50 - 20) );

  303.     line-height: calc( 1.5em + 0.2 * (100vw - 50rem) / (20 - 50) );

  304.     margin-left: 0;

  305.   }

  306.   h1 {

  307.     font-size: calc( 1.9rem + 1.5 * (100vw - 20rem) / (50 - 20) );

  308.     line-height: calc( 1.2em + 0.2 * (100vw - 50rem) / (20 - 50) );

  309.   }

  310.   h2 {

  311.     font-size: calc( 1.5rem + 1.5 * (100vw - 20rem) / (50 - 20) );

  312.     line-height: calc( 1.3em + 0.2 * (100vw - 50rem) / (20 - 50) );

  313.   }

  314.   h3 {

  315.     font-size: calc( 1.35rem + 1.5 * (100vw - 20rem) / (50 - 20) );

  316.     line-height: calc( 1.4em + 0.2 * (100vw - 50rem) / (20 - 50) );

  317.   }

  318. }

  319. @media (min-width: 50em) {

  320.   /* The right part of the addition *must* be a

  321.      rem value. In this example we *could* change

  322.      the whole declaration to font-size:2.5rem,

  323.      but if our baseline value was not expressed

  324.      in rem we would have to use calc. */

  325.   p, li, pre, code, kbd, samp, var, tt, time, details, figcaption {

  326.     font-size: calc( 1rem + .6 * 1rem );

  327.     line-height: 1.5em;

  328.   }

  329.   p, li, pre, details {

  330.     margin-left: 3rem;

  331.   }

  332.   h1 {

  333.     font-size: calc( 1.9rem + 1.5 * 1rem );

  334.     line-height: 1.2em;

  335.   }

  336.   h2 {

  337.     font-size: calc( 1.5rem + 1.5 * 1rem );

  338.     line-height: 1.3em;

  339.   }

  340.   h3 {

  341.     font-size: calc( 1.35rem + 1.5 * 1rem );

  342.     line-height: 1.4em;

  343.   }

  344.   figure img {

  345.     max-width: 500px;

  346.     max-height: 500px;

  347.   }

  348. }

  349. 

  350. figure.unsquared {

  351.   margin-bottom: 1.5rem;

  352. }

  353.   figure.unsquared img {

  354.     height: inherit;

  355.   }

  356. 

  357. 

  358. 

  359. @media print {

  360.   body { font-size: 100%; }

  361.   a:after { content: " (" attr(href) ")"; }

  362.   a, a:link, a:visited, a:after {

  363.     text-decoration: underline;

  364.     text-shadow: none !important;

  365.     background-image: none !important;

  366.     background: white;

  367.     color: black;

  368.   }

  369.   abbr[title] { border-bottom: 0; }

  370.   abbr[title]:after { content: " (" attr(title) ")"; }

  371.   img { page-break-inside: avoid; }

  372.   @page { margin: 2cm .5cm; }

  373.   h1, h2, h3 { page-break-after: avoid; }

  374.   p3 { orphans: 3; widows: 3; }

  375.   img {

  376.     max-width: 250px !important;

  377.     max-height: 250px !important;

  378.   }

  379.   nav, aside { display: none; }

  380. }

  381. 

  382. ul.with_columns {

  383.   column-count: 1;

  384. }

  385. @media (min-width: 20em) {

  386.   ul.with_columns {

  387.     column-count: 2;

  388.   }

  389. }

  390. @media (min-width: 50em) {

  391.   ul.with_columns {

  392.     column-count: 3;

  393.   }

  394. }

  395. ul.with_two_columns {

  396.   column-count: 1;

  397. }

  398. @media (min-width: 20em) {

  399.   ul.with_two_columns {

  400.     column-count: 1;

  401.   }

  402. }

  403. @media (min-width: 50em) {

  404.   ul.with_two_columns {

  405.     column-count: 2;

  406.   }

  407. }

  408. 

  409. .gallery {

  410.   display: flex;

  411.   flex-wrap: wrap;

  412.   justify-content: space-around;

  413. }

  414.   .gallery figure img {

  415.     margin-left: 1rem;

  416.     margin-right: 1rem;

  417.   }

  418.   .gallery figure figcaption {

  419.     font-family: 'ConcourseT3'

  420.   }

  421. 

  422. footer {

  423.   font-family: 'ConcourseT3';

  424.   display: flex;

  425.   flex-direction: column;

  426.   border-top: 3px solid white;

  427.   padding: 4rem 0;

  428.   background-color: #07486c;

  429.   color: white;

  430. }

  431.   footer > * {

  432.     max-width: 50rem;

  433.     margin: 0 auto;

  434.   }

  435.   footer a {

  436.     color: #f1c40f;

  437.   }

  438.   footer .avatar {

  439.     width: 200px;

  440.     height: 200px;

  441.     border-radius: 50%;

  442.     float: left;

  443.     -webkit-shape-outside: circle();

  444.     shape-outside: circle();

  445.     margin-right: 2rem;

  446.     padding: 2px 5px 5px 2px;

  447.     background: white;

  448.     border-left: 1px solid #f1c40f;

  449.     border-top: 1px solid #f1c40f;

  450.     border-right: 5px solid #f1c40f;

  451.     border-bottom: 5px solid #f1c40f;

  452.   }

  453. </style>

  454. 

  455. <h1>

  456.   <span><a id="jumper" href="#jumpto" title="Un peu perdu ?">?</a></span>

  457.   Giving better code reviews (archive)

  458.   <time>Pour la pérennité des contenus liés. Non-indexé, retrait sur simple email.</time>

  459. </h1>

  460. <section>

  461.   <article>

  462.     <h3><a href="https://medium.com/@mrjoelkemp/giving-better-code-reviews-16109e0fdd36">Source originale du contenu</a></h3>

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

  464. 

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

  466. 

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

  468. 

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

  470. 

  471. <h4 name="b0da" id="b0da" class="graf--h4 graf-after--p">Phase 1: The light pass</h4>

  472. 

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

  474. 

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

  476. 

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

  478. 

  479. <h4 name="ace7" id="ace7" class="graf--h4 graf-after--p">Phase 2: The contextual pass</h4>

  480. 

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

  482. 

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

  484. 

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

  486. 

  487. <p name="773f" id="773f" class="graf--p graf-after--p">In this contextual phase, we’re looking for the following:</p>

  488. 

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

  490. 

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

  492. 

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

  494. 

  495. <h4 name="b245" id="b245" class="graf--h4 graf-after--p">But this diff will take forever to review</h4>

  496. 

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

  498. 

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

  500. 

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

  502. 

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

  504. 

  505. <h4 name="1d8f" id="1d8f" class="graf--h4 graf-after--p">Can we do even better?</h4>

  506. 

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

  508. 

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

  510.   </article>

  511. </section>

  512. 

  513. 

  514. <nav id="jumpto">

  515.   <p>

  516.     <a href="/david/blog/">Accueil du blog</a> |

  517.     <a href="https://medium.com/@mrjoelkemp/giving-better-code-reviews-16109e0fdd36">Source originale</a> |

  518.     <a href="/david/stream/2019/">Accueil du flux</a>

  519.   </p>

  520. </nav>

  521. 

  522. <footer>

  523.   <div>

  524.     <img src="/static/david/david-larlet-avatar.jpg" loading="lazy" class="avatar" width="200" height="200">

  525.     <p>

  526.       Bonjour/Hi!

  527.       Je suis <a href="/david/" title="Profil public">David&nbsp;Larlet</a>, je vis actuellement à Montréal et j’alimente cet espace depuis 15 ans. <br>

  528.       Si tu as apprécié cette lecture, n’hésite pas à poursuivre ton exploration. Par exemple via les <a href="/david/blog/" title="Expériences bienveillantes">réflexions bimestrielles</a>, la <a href="/david/stream/2019/" title="Pensées (dés)articulées">veille hebdomadaire</a> ou en t’abonnant au <a href="/david/log/" title="S’abonner aux publications via RSS">flux RSS</a> (<a href="/david/blog/2019/flux-rss/" title="Tiens c’est quoi un flux RSS ?">so 2005</a>).

  529.     </p>

  530.     <p>

  531.       Je m’intéresse à la place que je peux avoir dans ce monde. En tant qu’humain, en tant que membre d’une famille et en tant qu’associé d’une coopérative. De temps en temps, je fais aussi des <a href="https://github.com/davidbgk" title="Principalement sur Github mais aussi ailleurs">trucs techniques</a>. Et encore plus rarement, <a href="/david/talks/" title="En ce moment je laisse plutôt la place aux autres">j’en parle</a>.

  532.     </p>

  533.     

  534.     <p>

  535.       Voici quelques articles choisis :

  536.       <a href="/david/blog/2019/faire-equipe/" title="Accéder à l’article complet">Faire équipe</a>,

  537.       <a href="/david/blog/2018/bivouac-automnal/" title="Accéder à l’article complet">Bivouac automnal</a>,

  538.       <a href="/david/blog/2018/commodite-effondrement/" title="Accéder à l’article complet">Commodité et effondrement</a>,

  539.       <a href="/david/blog/2017/donnees-communs/" title="Accéder à l’article complet">Des données aux communs</a>,

  540.       <a href="/david/blog/2016/accompagner-enfant/" title="Accéder à l’article complet">Accompagner un enfant</a>,

  541.       <a href="/david/blog/2016/senior-developer/" title="Accéder à l’article complet">Senior developer</a>,

  542.       <a href="/david/blog/2016/illusion-sociale/" title="Accéder à l’article complet">L’illusion sociale</a>,

  543.       <a href="/david/blog/2016/instantane-scopyleft/" title="Accéder à l’article complet">Instantané Scopyleft</a>,

  544.       <a href="/david/blog/2016/enseigner-web/" title="Accéder à l’article complet">Enseigner le Web</a>,

  545.       <a href="/david/blog/2016/simplicite-defaut/" title="Accéder à l’article complet">Simplicité par défaut</a>,

  546.       <a href="/david/blog/2016/minimalisme-esthetique/" title="Accéder à l’article complet">Minimalisme et esthétique</a>,

  547.       <a href="/david/blog/2014/un-web-omni-present/" title="Accéder à l’article complet">Un web omni-présent</a>,

  548.       <a href="/david/blog/2014/manifeste-developpeur/" title="Accéder à l’article complet">Manifeste de développeur</a>,

  549.       <a href="/david/blog/2013/confort-convivialite/" title="Accéder à l’article complet">Confort et convivialité</a>,

  550.       <a href="/david/blog/2013/testament-numerique/" title="Accéder à l’article complet">Testament numérique</a>,

  551.       et <a href="/david/blog/" title="Accéder aux archives">bien d’autres…</a>

  552.     </p>

  553.     <p>

  554.       On peut <a href="mailto:david%40larlet.fr" title="Envoyer un courriel">échanger par courriel</a>. Si éventuellement tu souhaites que l’on travaille ensemble, tu devrais commencer par consulter le <a href="http://larlet.com">profil dédié à mon activité professionnelle</a> et/ou contacter directement <a href="http://scopyleft.fr/">scopyleft</a>, la <abbr title="Société coopérative et participative">SCOP</abbr> dont je fais partie depuis six ans. Je recommande au préalable de lire <a href="/david/blog/2018/cout-site/" title="Attention ce qui va suivre peut vous choquer">combien coûte un site</a> et pourquoi je suis plutôt favorable à une <a href="/david/pro/devis/" title="Discutons-en !">non-demande de devis</a>.

  555.     </p>

  556.     <p>

  557.       Je ne traque pas ta navigation mais mon

  558.       <abbr title="Alwaysdata, 62 rue Tiquetonne 75002 Paris, +33.184162340">hébergeur</abbr>

  559.       conserve des logs d’accès.

  560.     </p>

  561.   </div>

  562. </footer>

  563. <script type="text/javascript">

  564.   ;(_ => {

  565.     const jumper = document.getElementById('jumper')

  566.     jumper.addEventListener('click', e => {

  567.       e.preventDefault()

  568.       const anchor = e.target.getAttribute('href')

  569.       const targetEl = document.getElementById(anchor.substring(1))

  570.       targetEl.scrollIntoView({behavior: 'smooth'})

  571.     })

  572.   })()

  573. </script>