Refactoring Day 26 : Remove Double Negative
Today’s refactoring comes from Fowler’s refactoring catalog and can be found here.
This refactoring is pretty simple to implement although I find it in many codebases that severely hurts readability and almost always conveys incorrect intent. This type of code does the most damage because of the assumptions made on it. Assumptions lead to incorrect maintenance code written, which in turn leads to bugs. Take the following example:
1: public class Order
<p>
<!--CRLF-->
</p>
<pre><span class="lnum"> 2:</span> {</pre>
<p>
<!--CRLF-->
</p>
<pre><span class="lnum"> 3:</span> <span class="kwrd">public</span> <span class="kwrd">void</span> Checkout(IEnumerable<Product> products, Customer customer)</pre>
<p>
<!--CRLF-->
</p>
<pre><span class="lnum"> 4:</span> {</pre>
<p>
<!--CRLF-->
</p>
<pre><span class="lnum"> 5:</span> <span class="kwrd">if</span> (!customer.IsNotFlagged)</pre>
<p>
<!--CRLF-->
</p>
<pre><span class="lnum"> 6:</span> {</pre>
<p>
<!--CRLF-->
</p>
<pre><span class="lnum"> 7:</span> <span class="rem">// the customer account is flagged</span></pre>
<p>
<!--CRLF-->
</p>
<pre><span class="lnum"> 8:</span> <span class="rem">// log some errors and return</span></pre>
<p>
<!--CRLF-->
</p>
<pre><span class="lnum"> 9:</span> <span class="kwrd">return</span>;</pre>
<p>
<!--CRLF-->
</p>
<pre><span class="lnum"> 10:</span> }</pre>
<p>
<!--CRLF-->
</p>
<pre><span class="lnum"> 11:</span>  </pre>
<p>
<!--CRLF-->
</p>
<pre><span class="lnum"> 12:</span> <span class="rem">// normal order processing</span></pre>
<p>
<!--CRLF-->
</p>
<pre><span class="lnum"> 13:</span> }</pre>
<p>
<!--CRLF-->
</p>
<pre><span class="lnum"> 14:</span> }</pre>
<p>
<!--CRLF-->
</p>
<pre><span class="lnum"> 15:</span>  </pre>
<p>
<!--CRLF-->
</p>
<pre><span class="lnum"> 16:</span> <span class="kwrd">public</span> <span class="kwrd">class</span> Customer</pre>
<p>
<!--CRLF-->
</p>
<pre><span class="lnum"> 17:</span> {</pre>
<p>
<!--CRLF-->
</p>
<pre><span class="lnum"> 18:</span> <span class="kwrd">public</span> <span class="kwrd">decimal</span> Balance { get; <span class="kwrd">private</span> set; }</pre>
<p>
<!--CRLF-->
</p>
<pre><span class="lnum"> 19:</span>  </pre>
<p>
<!--CRLF-->
</p>
<pre><span class="lnum"> 20:</span> <span class="kwrd">public</span> <span class="kwrd">bool</span> IsNotFlagged</pre>
<p>
<!--CRLF-->
</p>
<pre><span class="lnum"> 21:</span> {</pre>
<p>
<!--CRLF-->
</p>
<pre><span class="lnum"> 22:</span> get { <span class="kwrd">return</span> Balance < 30m; }</pre>
<p>
<!--CRLF-->
</p>
<pre><span class="lnum"> 23:</span> }</pre>
<p>
<!--CRLF-->
</p>
<pre><span class="lnum"> 24:</span> }</pre>
<p>
<!--CRLF--></div> </div>
<p>
As you can see the double negative here is difficult to read because we have to figure out what is positive state of the two negatives. The fix is very easy. If we don’t have a positive test, add one that does the double negative assertion for you rather than make sure you get it correct.
</p>
<div class="csharpcode-wrapper">
<div class="csharpcode">
<pre><span class="lnum"> 1:</span> <span class="kwrd">public</span> <span class="kwrd">class</span> Order</pre>
<p>
<!--CRLF-->
</p>
<pre><span class="lnum"> 2:</span> {</pre>
<p>
<!--CRLF-->
</p>
<pre><span class="lnum"> 3:</span> <span class="kwrd">public</span> <span class="kwrd">void</span> Checkout(IEnumerable<Product> products, Customer customer)</pre>
<p>
<!--CRLF-->
</p>
<pre><span class="lnum"> 4:</span> {</pre>
<p>
<!--CRLF-->
</p>
<pre><span class="lnum"> 5:</span> <span class="kwrd">if</span> (customer.IsFlagged)</pre>
<p>
<!--CRLF-->
</p>
<pre><span class="lnum"> 6:</span> {</pre>
<p>
<!--CRLF-->
</p>
<pre><span class="lnum"> 7:</span> <span class="rem">// the customer account is flagged</span></pre>
<p>
<!--CRLF-->
</p>
<pre><span class="lnum"> 8:</span> <span class="rem">// log some errors and return</span></pre>
<p>
<!--CRLF-->
</p>
<pre><span class="lnum"> 9:</span> <span class="kwrd">return</span>;</pre>
<p>
<!--CRLF-->
</p>
<pre><span class="lnum"> 10:</span> }</pre>
<p>
<!--CRLF-->
</p>
<pre><span class="lnum"> 11:</span>  </pre>
<p>
<!--CRLF-->
</p>
<pre><span class="lnum"> 12:</span> <span class="rem">// normal order processing</span></pre>
<p>
<!--CRLF-->
</p>
<pre><span class="lnum"> 13:</span> }</pre>
<p>
<!--CRLF-->
</p>
<pre><span class="lnum"> 14:</span> }</pre>
<p>
<!--CRLF-->
</p>
<pre><span class="lnum"> 15:</span>  </pre>
<p>
<!--CRLF-->
</p>
<pre><span class="lnum"> 16:</span> <span class="kwrd">public</span> <span class="kwrd">class</span> Customer</pre>
<p>
<!--CRLF-->
</p>
<pre><span class="lnum"> 17:</span> {</pre>
<p>
<!--CRLF-->
</p>
<pre><span class="lnum"> 18:</span> <span class="kwrd">public</span> <span class="kwrd">decimal</span> Balance { get; <span class="kwrd">private</span> set; }</pre>
<p>
<!--CRLF-->
</p>
<pre><span class="lnum"> 19:</span>  </pre>
<p>
<!--CRLF-->
</p>
<pre><span class="lnum"> 20:</span> <span class="kwrd">public</span> <span class="kwrd">bool</span> IsFlagged</pre>
<p>
<!--CRLF-->
</p>
<pre><span class="lnum"> 21:</span> {</pre>
<p>
<!--CRLF-->
</p>
<pre><span class="lnum"> 22:</span> get { <span class="kwrd">return</span> Balance >= 30m; }</pre>
<p>
<!--CRLF-->
</p>
<pre><span class="lnum"> 23:</span> }</pre>
<p>
<!--CRLF-->
</p>
<pre><span class="lnum"> 24:</span> }</pre>
<p>
<!--CRLF--></div> </div>
<p>
<em><span style="font-size: xx-small">This is part of the 31 Days of Refactoring series. For a full list of Refactorings please see the <a href="/blogs/sean_chambers/archive/2009/08/01/31-days-of-refactoring.aspx" target="_blank">original introductory post</a>.</span></em>
</p>