Practical Refactoring
I've done a few presentations on refactoring in ColdFusion and in those presentations, I show a lot of code samples towards the end. No matter how eloquent I am in the first part of the presentation, the light always goes on for the audience when I show code. If you think about it, it makes sense, why try to explain concrete principles in abstract terms?
I do my refactoring in sweeps. A sweep is a cycle through the code in which I make incremental improvements. At the end of a sweep, the code should (it better) still function as well as it did pre-sweep. By breaking up the refactoring up into small sweeps, we can stay focused and keep out of the weeds.
In this article, we are going to look at some code that could use a refactor. Then we'll make a few sweeps through the code to tidy it up a bit. Along the way, I'll explain why I am refactoring out a certain piece. Keep in mind, we are all still learning. None of this is meant as Final Law of Software Design. I'm always refining my viewpoints and learning new things so I'll probably disagree with something I've done here at some point in the future. In the end, as long as this article is thought provoking for you, we've both won. Can Ya Dig?
First, here is the code sample. Scan the code and fix in your mind the general idea of what is going on. We'll pick back up on the other side.
The Original Code
<cffunction name="onError" output="true">
<cfargument name="exception" required="true"/>
<cfargument name="eventName" required="true"/>
<cfif compareNoCase(trim(arguments.eventName),"onSessionEnd") And compareNoCase(trim(arguments.eventName),"onApplicationEnd")>
<cfif compareNoCase( arguments.Exception.RootCause.Type, "coldfusion.runtime.AbortException") >
<cfoutput>
<CFIF isDebugMode() is false>
<cfinclude template="/errortemplate.htm"/>
<CFELSE>
<h2>An unexpected error has occurred.</h2>
<p>Error event: #arguments.eventName#</p>
<p>Error details:</p>
<cfdump var="#arguments.exception#"/>
</CFIF>
<cflog file="#this.name#" type="error" text="Event name: #arguments.eventName#"/>
<cflog file="#this.name#" type="error" text="Message: #arguments.exception.message#"/>
</cfoutput>
</cfif>
<cfelseif not compareNoCase(arguments.eventName,"onApplicationEnd")>
<cflog file="#this.name#" type="Information" text="Application #this.name# ended."/>
<cflog file="#this.name#" type="error" text="Event name: #arguments.eventName#"/>
<cflog file="#this.name#" type="error" text="Message: #arguments.exception.message#"/>
</cfif>
</cffunction>
Summary
Ok, answer to yourself the following questions:
- Is the code right?
- Do you know the purpose of the code?
- Is it easy to understand?
The first question is a bit of a slippery slope. In my book 'Right' and 'Wrong' are determined by whether or not the code works. The code above actually works just fine. Compiled down to Java Bytecode, it happily processes errors with no defects, so it is 'Right'. When dealing with fuzzy subjects like code-cleanliness and proper organization, you'll find many more opinions than hard/fast rules. Clean Code is like Good Art, you know what you like when you see it. So stay away from calling other developers' code 'Wrong'. It only makes people defensive and defensive minds are not in learning mode.
Now that we've gotten past that, did you understand the purpose of the code? If you said "this code handles errors", give yourself 2 points. (The function name helped, didn't it?)
Was the code easy to understand? Can you verbalize what the code is doing? Try... no seriously, read this code from top to bottom. Can you do it? Did you have to reread parts of it?
It is a known fact that code we write personally instintively makes more sense to us than code other people write. This holds true even months after the code has been written. Good programmers write code that can be easily be understood by others. We are going to refactor this code to be more easily read.
First Analysis
A few easy things to point out... compare() and compareNoCase() are two functions that work opposite of how we read. If you use compare() to compare two identical strings, you get a return of 0, which ColdFusion treats as false, meaning we have to reverse our boolean logic in conditionals to accomodate this. Some people will tell you that compare() is faster than using an operator like IS or IS NOT. They might be right, I'm going to refactor the use of compare() and compareNoCase() out of the code anyways. If it turns out we needed the extra 'performance' of these functions, we should have implemented this part of the site in Hardware.
Next, the case is mixed inappropriately. Some operators are lower case, some are mixed case. Some of the conditionals are upper case, some are lower case. We'll normalize this and get it all looking consistant.
Also, this code has nested conditionals. Some times the only way to represent logical flow is to nest conditionals though most often it can be simplified for better readability. In this case, it looks like we truly care about a few conditions so I'll look at untangling the nest as well.
I'll put a few comments in as well, since that will help us remember what we were thinking in the middle of the refactor. Without further preamble...
First Refactor
<cffunction name="onError" output="true">
<cfargument name="exception" required="true"/>
<cfargument name="eventName" required="true"/>
<!--- Exit conditions --->
<cfif arguments.Exception.RootCause.Type IS "coldfusion.runtime.AbortException">
<cfreturn />
</cfif>
<cfif trim(arguments.eventName) IS "onSessionEnd">
<cfreturn />
</cfif>
<!--- Log and Exit --->
<cfif trim(arguments.eventName) IS "onApplicationEnd">
<cflog file="#this.name#" type="Information" text="Application #this.name# ended."/>
<cflog file="#this.name#" type="error" text="Event name: #arguments.eventName#"/>
<cflog file="#this.name#" type="error" text="Message: #arguments.exception.message#"/>
<cfreturn />
</cfif>
<!--- use error template file or print to screen --->
<cfoutput>
<cfif isDebugMode() is false>
<cfinclude template="/errortemplate.htm"/>
<cfelse>
<h2>An unexpected error has occurred.</h2>
<p>Error event: #arguments.eventName#</p>
<p>Error details:</p>
<cfdump var="#arguments.exception#"/>
</cfif>
<cflog file="#this.name#" type="error" text="Event name: #arguments.eventName#"/>
<cflog file="#this.name#" type="error" text="Message: #arguments.exception.message#"/>
</cfoutput>
</cffunction>
Second Analysis
Ok, That is a pretty good start. We got rid of compare() and compareNoCase() so the code reads a little nicer. We got rid of the nested conditionals so we don't have to keep track of several layers of boolean logic and we cleaned up the inconsistent use of case.
Another thing that I try to stay away from is using negative boolean logic. See where we check for the isDebugMode() function? I'd much rather have the TRUE case up top and have the negative case be the ELSE portion.
Also, some of the logging is duplicated. Looks like someone was Copy/Pasting :). Let's clean that up as well.
Lastly, we are going to put in good comments all the way down the line and use appropriate whitespace for readability.
Second Refactor
<cffunction name="onError" output="true">
<cfargument name="exception" required="true"/>
<cfargument name="eventName" required="true"/>
<!--- We don't want to deal with the cflocation as an error, so bail --->
<cfif arguments.Exception.RootCause.Type IS "coldfusion.runtime.AbortException">
<cfreturn />
</cfif>
<!--- Lets deal with events next. --->
<!--- We don't want to deal with the onSessionEnd as an error, so bail --->
<cfif trim(arguments.eventName) IS "onSessionEnd">
<cfreturn />
</cfif>
<!--- From this point onward, we want to log the errors --->
<cflog file="#this.name#" type="error" text="Event name: #arguments.eventName#"/>
<cflog file="#this.name#" type="error" text="Message: #arguments.exception.message#"/>
<!--- Log as Information and Bail --->
<cfif trim(arguments.eventName) IS "onApplicationEnd">
<cflog file="#this.name#" type="Information" text="Application #this.name# ended."/>
<cfreturn />
</cfif>
<!--- Now, let's deal with explicit types we want to handle... --->
<!--- If we get here, we have something to handle --->
<cfif isDebugMode() is true>
<!--- print the error to the screen --->
<cfoutput>
<h2>An unexpected error has occurred.</h2>
<p>Error event: #arguments.eventName#</p>
<p>Error details:</p>
<cfdump var="#arguments.exception#"/>
</cfoutput>
<cfelse>
<!--- use the nice error handler --->
<cfinclude template="/errortemplate.htm"/>
</cfif>
</cffunction>
Final Analysis
That looks even better. Now that our conditionals use positive boolean logic they are much easier to read. We also were able to remove duplicate logging calls. Not all code duplication is bad, of course, but when we can get rid of it and improve the clarity and readability, we all win.
The comments are also very descriptive and tell us the intent and reasons for the code. This way, other developers can quickly understand what we were trying to do, even if it isn't working properly.
Just for fun, I'm going to paste the finished code below without comments and whitespace. Read through the code and notice how it reads more like English language.
Final Code - No Comments/No Whitespace
<cffunction name="onError" output="true">
<cfargument name="exception" required="true"/>
<cfargument name="eventName" required="true"/>
<cfif arguments.Exception.RootCause.Type IS "coldfusion.runtime.AbortException">
<cfreturn />
</cfif>
<cfif trim(arguments.eventName) IS "onSessionEnd">
<cfreturn />
</cfif>
<cflog file="#this.name#" type="error" text="Event name: #arguments.eventName#"/>
<cflog file="#this.name#" type="error" text="Message: #arguments.exception.message#"/>
<cfif trim(arguments.eventName) IS "onApplicationEnd">
<cflog file="#this.name#" type="Information" text="Application #this.name# ended."/>
<cfreturn />
</cfif>
<cfif isDebugMode() is true>
<cfoutput>
<h2>An unexpected error has occurred.</h2>
<p>Error event: #arguments.eventName#</p>
<p>Error details:</p>
<cfdump var="#arguments.exception#"/>
</cfoutput>
<cfelse>
<cfinclude template="/errortemplate.htm"/>
</cfif>
</cffunction>
Not to shabby, yeah? The code works just like the original sample, but it is much easier to understand the flow of the function. Also, by doing our refactoring in sweeps, we simplified the code in stages, reducing the chance we'll miss something. Now go to the last sample and read it aloud. Notice how it feels natural when you read it? This code will definitely be easier to maintain in the future, won't it? There are lots of other principles to keep in mind when refactoring. We'll look at a few more in the next article.
Agree? Disagree? Tell me all about it in the comments.
P.S. Do you have code you'd like to see refactored?
If you have a code sample you would like me to refactor, send it to me using the Contact Form on this blog. Please try to keep the code chunk below 100 lines as that seems to be the limit of clarity for a blog article.
