So You Want Cleaner Code? Nesting Conditionals and Booleans

This post continues the infrequent series on Clean Code examples. If you are enjoying this series, come to CFUnited 2010 and CF.Objective() 2010 to catch my presentation, Making Bad Code Good- Part 2, a live version of this series.

The main idea here is to provide practical examples of working code that can be written in a cleaner fashion. You may agree or disagree with what I've written, and I want to hear from you either way in the comments.

If you have a code sample you'd like to see refactored, send it to me through email (if you have my email already) or through the Contact Me form on this blog.

Code Sample

view plain print about
1<cffunction name="execute" access="public" output="false" returntype="boolean">
2    <cfargument name="attributes" required="true"/>
3    <cfset var local = structNew() />
4    <cfset local.customerGateway = createObject("component","model.customerGateway").init() />
5    <cfif structKeyExists( client, "customerID" ) AND val( client.customerID ) GT 0>
6        <cfif val( arguments.customerGW.getByID( val(client.customerID ) ).getChallengeQuestionID() ) GT 0>
7            <cfset local.result = "true" />
8        </cfif>
9    <cfelseif structKeyExists( arguments.attributes, "ChallengeAnswer") AND
10                structKeyExists( arguments.attributes, "ChallengeQuestionID") >

11        <cfif structKeyExists( arguments.attributes, "ChallengeQuestionID") AND val( attributes.ChallengeQuestionID ) GT 0>
12            <cfif structKeyExists( arguments.attributes, "ChallengeAnswer") AND len( trim( attributes.ChallengeAnswer ) ) GT 0>
13                <cfset local.cust =    arguments.customerGW.getById( client.customerID ) /> />

14                <cfset local.cust.setChallengeQuestionID( attributes.ChallengeQuestionID ) />
15                <cfset local.cust.setChallengeAnswer( attributes.ChallengeAnswer ) />
16                <cfset arguments.customerGW.save( cust ) />
17                <cfset local.result = "true" />
18            <cfelse>
19                <cfset local.result = "false" />
20                <cfset attributes.userMessage= "Please select a security question and provide an answer." />
21            </cfif>
22        </cfif>
23    <cfelse>    
24        <cfset local.result = "false" />
25    </cfif>        
26    <cfreturn local.result />
27</cffunction>

Read through the entire example and ask yourself the following questions:

  • What is the purpose of this code block? What are you using for your inferences?
  • Is it clear what this code is doing? Why or why not?
  • Can you spot the subtle bug in the code?

Ok, I tricked you, there is no subtle bug in the code. This code sample actually processes like it was intended to and does not have any bugs in it.

Was this code easy to understand? Were you able to follow along with the processing and can you articulate the correct conditions at each branch? Why or why not?

Would you believe that 33% of this code sample is either a <cfif>, <cfelse>, <cfelseif> or a </cfif> tag? Furthermore, at some points in the code, we are nested three levels deep. This confuses me more than anything else in the code, so we'll start there.

Whenever we refactor, we spend time learning the process expressed by the source code. Since it takes time to gain this learning, it makes sense to stick comments in the relevant sections as we go.

Also, when refactoring, it is very easy to get distracted and make too many changes. Sure there are plenty of things we could change, and we'll get to it one step at a time. Making too many changes at once is non-scientific and leads to a non-working hot mess. Comprende? Let's take a look at a refactored example:

Minimizing Nested Conditionals

view plain print about
1<cffunction name="execute" access="public" output="false" returntype="boolean">
2    <cfargument name="attributes" required="true"/>
3    <cfset var local = structNew() />
4    <cfset local.customerGateway = createObject("component","model.customerGateway").init() />
5    <cfset local.result = "false" />
6    <!--- logged in and has challenge question --->
7    <cfif structKeyExists( client, "customerID" ) AND val( client.customerID ) GT 0
8    AND val( local.customerGateway.getByID( val(client.customerID ) ).getChallengeQuestionID() ) GT 0>

9        <cfset local.result = "true" />
10    <!--- challenge Info Submitted --->
11    <cfelseif structKeyExists( arguments.attributes, "ChallengeAnswer")
12    AND structKeyExists( arguments.attributes, "ChallengeQuestionID")>

13        <!--- validate challenge info --->
14        <cfif structKeyExists( arguments.attributes, "ChallengeQuestionID")
15        AND val( attributes.ChallengeQuestionID ) GT 0
16        AND structKeyExists( arguments.attributes, "ChallengeAnswer")
17        AND len( trim( attributes.ChallengeAnswer ) ) GT 0    >

18            <!--- save challenge answer --->
19            <cfset local.cust =    local.customerGateway.getById( client.customerID ) /> />

20            <cfset local.cust.setChallengeQuestionID( attributes.ChallengeQuestionID ) />
21            <cfset local.cust.setChallengeAnswer( attributes.ChallengeAnswer ) />
22            <cfset local.customerGateway.save( local.cust ) />
23            <cfset local.result = "true" />
24        <cfelse>
25            <!--- Deal with invalid data --->
26            <cfset attributes.userMessage= "Please select a security question and provide an answer." />
27        </cfif>    
28    </cfif>        
29    <cfreturn local.result />
30</cffunction>
Better. We got rid of some of the nesting, removed an extra variable and left ourself notes of what this thing actually does. We've definitely improved the code and should be proud of ourselves for sticking to the game plan. Read over the second code sample once more and ask yourself the same questions again:
  • What is the purpose of this code block? What are you using for your inferences?
  • Is it clear what this code is doing? Why or why not?

At this point, we are being overrun with complex conditionals. Sure we have comments to tell us that structKeyExists( arguments.attributes, "ChallengeAnswer") AND structKeyExists( arguments.attributes, "ChallengeQuestionID") actually means challenge info was submitted, but the code itself still isn't very clear. Let's try to simplify the complex boolean logic.

Removing Complex Boolean Logic

view plain print about
1<cffunction name="execute" access="public" output="false" returntype="boolean">
2    <cfargument name="attributes" required="true"/>
3    <cfset var local = structNew() />
4    <cfset local.customerGateway = createObject("component","model.customerGateway").init() />
5    <cfset local.result = "false" />
6    <!--- logged in and has challenge question --->
7    <cfif isLoggedIn() AND hasChallengeQuestionSet( attributes, local.customerGateway )>
8        <cfset local.result = "true" />
9    <!--- challenge Info Submitted --->
10    <cfelseif challengeInfoSubmitted( arguments.attributes ) >
11        <!--- validate challenge info --->
12        <cfif isValidChallengeInfo( arguments.attributes )>
13            <!--- save challenge answer --->
14        <cfset local.cust =    local.customerGateway.getById( client.customerID ) /> />

15        <cfset local.cust.setChallengeQuestionID( attributes.ChallengeQuestionID ) />
16        <cfset local.cust.setChallengeAnswer( attributes.ChallengeAnswer ) />
17        <cfset local.customerGateway.save( local.cust ) />
18            <cfset local.result = "true" />
19        <cfelse>
20            <!--- Deal with invalid data --->
21            <cfset attributes.userMessage= "Please select a security question and provide an answer." />
22        </cfif>    
23    </cfif>        
24    <cfreturn local.result />
25</cffunction>
Give that a read. Have your answers changed?
  • What is the purpose of this code block? What are you using for your inferences?
  • Is it clear what this code is doing? Why or why not?
Where did we stick all of the boolean logic? Are there magic code pixies inside the computer sprinkling simplicity dust? Of course not! We moved the boolean bits into their own separate functions so we can better express the intent of the code, not the inner workings of the code. All the processing in our execute method is at the same level. The lower level details, the implementation, is tucked away in another level. Thus:

view plain print about
1<cfset challengeInfoSubmitted() />

simply calls:

view plain print about
1<cffunction name="challengeInfoSubmitted" output="false" access="private" returntype="boolean">
2    <cfargument name="attributes" type="struct" required="true"/>
3    <cfreturn structKeyExists( arguments.attributes, "ChallengeAnswer") AND structKeyExists( arguments.attributes, "ChallengeQuestionID") />
4</cffunction>

and this:

view plain print about
1<cfset isValidChallengeInfo() />

simply calls:

view plain print about
1<cffunction name="isValidChallengeInfo" output="false" access="private" returntype="boolean">
2    <cfargument name="attributes" type="struct" required="true"/>
3    <cfreturn structKeyExists( arguments.attributes, "ChallengeQuestionID")
4                        AND val( attributes.ChallengeQuestionID ) GT 0
5                        AND structKeyExists( arguments.attributes, "ChallengeAnswer")
6                        AND len( trim( attributes.ChallengeAnswer ) ) GT 0 />

7</cffunction>

Now what?

Sure you may agree this code is cleaner, but when do we have time to work on cleaning up code? Don't we all have jobs, deadlines and managers with budgets? Consider this:

If we spend much of our time maintaining applications, this means we are either fixing bugs, or bolting on yet another feature to an existing application. By design, we are always reading code, keeping the entire problem context in our brain while we try to figure out how the heck it works. By using the techniques above, cleaning up code as we go, we take advantage of the hard earned understanding of how a piece of code works. Now, you can fix the bug or add the new feature without having to keep a huge problem map in your brain (this especially helps when you are bombarded with emails, twitter alerts, IM, phone calls, and pesky managers with their pesky project plan questions :).

Just keep in mind these simple rules:

  • Focus on High Value changes leading to better code cleanliness and structure
  • Stay focused in your changes. Improve one concept at a time
  • Stay away from renaming variables, changing intentations, replacing tabs with spaces, or other personal preference changes. Leave that for when you have extra time
  • Reread your code samples aloud, you'll often be surprised by how hard this is. (You have more experience reading, since you started reading at 2, and coding at 20 )
  • Work smarter, not harder. Refactor for peace of mind.

This code isn't perfect yet and I bet you can improve the last code sample even more. If you want to have a go, email your code sample and explanation to me or use the contact form. I'll post it here. (Be sure to reference the title of this post in your email)

Related Blog Entries

There are no comments for this entry.

Add Comment Subscribe to Comments

1/11/10 11:40 AM # Posted By Justin Alpino

A lot of times I will set up conditional statements in my methods like the following, just for readability purposes:

<cfset local.challengeInfoSubmitted = structKeyExists( arguments.attributes, "ChallengeAnswer")
AND structKeyExists( arguments.attributes, "ChallengeQuestionID")>

Then in the course of using it, my code would like:

<cfif local.challengeInfoSubmitted >
...
</cfif>


I think it accomplishes (from a readability perspective) the same thing as breaking out the conditional check into it's own method.

Nice series btw.


1/11/10 11:43 AM # Posted By Dan Wilson

@JAlpino,

Thanks for the comment. I do that in places where it seems to make less semantic sense to have extra methods, like views and such.

Matter of fact, I'll be showing this technique in a later series. Glad to know others like to structure their code that way also.

DW


1/11/10 12:53 PM # Posted By Steve Bryant

Dan,

I sometimes like to create StructKeyHasVal() and StructKeyHasLen() functions as shortcuts for StructKeyExists with Val() or Len() respectively.


1/11/10 10:00 PM # Posted By Leigh

I totally agree with Justin. I think it increases not only readability, but clarity as well.

My preference is to also preface the variable names with "is", "has", etecetera. Then I can easily chain together a few (small) well-named, boolean variables in my conditionals. I find this approach provides a much better sense of what the code is doing, over a long series of structKeyExists() statements.

<cfset var IsLoggedIn = .....>
<cfset var HasChallengeQuest = .....>

<cfif IsLoggedIn AND HasChallengeQuest>
   ... then do something...
</cfif>


1/11/10 10:39 PM # Posted By Leigh

Oh.. and since @Dan had the smarts to raise the topic in the first place... I guess I agree with him too ;)


Add Comment Subscribe to Comments