davidbgk
/
larlet-fr-david-cache

title: Stop More Bugs with our Code Review Checklist
url: http://blog.fogcreek.com/increase-defect-detection-with-our-code-review-checklist-example/
hash_url: 3177652e7a1a4808cc303cb58246cbf3

<p>In our blog about effective <a href="https://blog.fogcreek.com/effective-code-reviews-9-tips-from-a-converted-skeptic" target="_new">code reviews</a>, we recommended the use of a checklist. Checklists are a great tool in code reviews &#8211; they ensure that reviews are consistently performed throughout your team. They’re also a handy way to ensure that common issues are identified and resolved.</p>
<p>Research by the Software Engineering Institute suggests that programmers make 15-20 common mistakes. So by adding such mistakes to a checklist, you can make sure that you spot them whenever they occur and help drive them out over time.</p>
<p>To get you started with a checklist, here’s a list of typical items:</p>
<h3>Code Review Checklist</h3>
<p><b>General</b></p>
<ul>
<li>Does the code work? Does it perform its intended function, the logic is correct etc.</li>
<li>Is all the code easily understood?</li>
<li>Does it conform to your agreed coding conventions? These will usually cover location of braces, variable and function names, line length, indentations, formatting, and comments.</li>
<li>Is there any redundant or duplicate code?</li>
<li>Is the code as modular as possible?</li>
<li>Can any global variables be replaced?</li>
<li>Is there any commented out code?</li>
<li>Do loops have a set length and correct termination conditions?</li>
<li>Can any of the code be replaced with library functions?</li>
<li>Can any logging or debugging code be removed?</li>
</ul>
<p><b>Security</b></p>
<ul>
<li>Are all data inputs checked (for the correct type, length, format, and range) and encoded?</li>
<li>Where third-party utilities are used, are returning errors being caught?</li>
<li>Are output values checked and encoded?</li>
<li>Are invalid parameter values handled?</li>
</ul>
<p><b>Documentation</b></p>
<ul><img src="https://blog.fogcreek.com/wp-content/uploads/2014/11/test-work-300x198.jpg" alt="integration testing" width="300" height="198" class="alignright size-medium wp-image-4572" align="right" style="margin-left: 15px; margin-bottom: 15px" srcset="https://blog.fogcreek.com/wp-content/uploads/2014/11/test-work-300x198.jpg 300w, https://blog.fogcreek.com/wp-content/uploads/2014/11/test-work-1024x678.jpg 1024w, https://blog.fogcreek.com/wp-content/uploads/2014/11/test-work.jpg 1280w" sizes="(max-width: 300px) 100vw, 300px" /></p>
<li>Do comments exist and describe the intent of the code?</li>
<li>Are all functions commented?</li>
<li>Is any unusual behavior or edge-case handling described?</li>
<li>Is the use and function of third-party libraries documented?</li>
<li>Are data structures and units of measurement explained?</li>
<li>Is there any incomplete code? If so, should it be removed or flagged with a suitable marker like &#8216;TODO&#8217;?</li>
</ul>
<p><b>Testing</b></p>
<ul>
<li>Is the code testable? i.e. don’t add too many or hide dependencies, unable to initialize objects, test frameworks can use methods etc.</li>
<li>Do tests exist and are they comprehensive? i.e. has at least your agreed on code coverage.</li>
<li>Do unit tests actually test that the code is performing the intended functionality?</li>
<li>Are arrays checked for ‘out-of-bound’ errors?</li>
<li>Could any test code be replaced with the use of an existing API?</li>
</ul>
<p>You’ll also want to add to this checklist any language-specific issues that can cause problems.</p>
<p>The checklist is deliberately not exhaustive of all issues that can arise. You don’t want a checklist, which is so long no-one ever uses it. It’s better to just cover the common issues. </p>
<h3>Optimize Your Checklist</h3>
<p>Using the checklist as a starting point, you should optimize it for your specific use-case. A great way to do this is to get your team to note the issues that arise during code reviews for a short time. With this data, you’ll be able to identify your team&#8217;s common mistakes, which you can then build into a custom checklist. Be sure to remove any items that don’t come up (you may wish to keep rarely occurring, yet critical items such as security related issues). </p>
<h3>Get Buy-in and Keep It Up To Date</h3>
<p>As a general rule, any items on the checklist should be specific and, if possible, something you can make a binary decision about. This helps to avoid inconsistency in judgments. It is also a good idea to share the list with your team and get their agreement on its content. Make sure to review the checklist periodically too, to check that each item is still relevant.</p>
<p>Armed with a great checklist, you can raise the number of defects you detect during code reviews. This will help you to drive up coding standards and avoid inconsistent code review quality.</p>