About Dan Wilson
Dan Wilson
Dan Wilson
Model-Glue:Gesture for Fun and Profit
Watch Video
Presentation Files
DataCurl LLC, My Company
2010 Speaking Engagements
Video Interviews
DZone Articles
So You Want Cleaner Code? Return Statement Execution
Posted By : Dan Wilson
December 30, 2009 12:05 PM
I'm going to start an 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
2 <cfreturn true />
3<cfelse>
4 <cfreturn false />
5</cfif>
This 5 liner example isn't too terrible, but we can certainly do better. The guiding principle in clean code is to make your code easy to understand for humans. In this case, the code isn't too obfuscated, but it is a little overly wordy.
The intent of the above code sample is to return whether FindKey.Recordcount is greater than 0. This is handled by the initial conditional branch evaluation:
This evaluation is already as simple as it can be, so let's attack the verbosity of the statement. Simple code is rarely measured in the smallest lines of code possible, but in this case we can shorten the code to clarify the intent.
The solution in this case is to take advantage of the return statement as an execution zone. We can put any evaluation we want to in the return statement, including boolean statements, function evaluations, anything we want really. After making this change to the original code sample, we can see the code is easier to understand.
Thoughts? Ideas? Opinions? Tell me all about it in the comments:
There are no comments for this entry.
Add Comment
Subscribe to Comments
I like this technique. Not only are you dropping from 5 lines of code to 1, but you've taught me something new: I have embedded functions in cfreturn before, but had no idea you could embed conditional statements. Cool!
Personally, I prefer the following:
<cfreturn (FindKey.Recordcount gt 0) />
I just think the parenthesis make it clearer that the conditional is being treated as a value.
That is probably just personal preference though. I like your approach. Brevity doesn't always enhance clarity, but I think it does in this case.
As this is a recordcount, I usually end up going even a little shorter. <cfreturn FindKey.recordcount /> If there are values (i.e. recordcount is greater than 0), then it evaluates to true, else it evaluates to false. For clarity within the code, I guess some people like to leave the "gt 0", but as this is a very basic statement, I usually just go without.
@Gareth,
You are right to say in this case we could leave off the comparison and the code would still work.
I personally leave the comparator in the statement when writing ColdFusion code because of the way ColdFusion interprets types and values.
ColdFusion tries really hard to interpret values and infer types. 0, false, "false", "no" all signify a boolean false. However there are some conditions where ColdFusion can not properly infer a boolean, such as an empty string.
If there is a chance the variable foo could contain an empty string, the following code will fail:
<cfif foo>
This is because ColdFusion does not interpret the empty string as either a true or a false, it just throws an exception.
While we can pretty much be assured of recordcount always having a number, I prefer to keep the comparator in the statement because I want my conventions to be understood and followed by other developers on the team. So I try to keep them rather simple. Purely my personal preference though...
DW
In those cases (where I am unsure of what value is being set), I agree and also use the comparator. Just personal preference for me to leave off the comparator where either, I am setting the value or I am sure of what value is being set to the variable.
In the foo example, if you happen to use <cfif foo gt 0> couldn't it still evaluate incorrectly, it just won't bomb like <cfif foo> e.g. if foo is "0", then that has length, but it equals 0, but it would return false as it's not greater than zero.
If I know something is coming back as a string, and I want to check for a value in my string, I'll still usually use the shortened method, but do something like <cfif Len( Trim( foo ) ) />
After reading your example, I want to disagree with 'shorter is better' in this case. Sure, cutting 5 lines of code down to 1 seems like a good idea short term. However, doesn't this in fact make your code more confusing for the majority of others who make work the code in the future.
a) what if you want to add the equivalent of a cfelseif
b) more complex decision making in the existing if/then return
c) people may misinterpret the true/false logic, complex decisions on GTE versus GT, etc.
Sure, shorter/less code in theory may be easier to maintain, but if the code becomes more complex you may be defeating the purpose. There is something to be said for the convenience and simplicity of the original logic "if a then do b, else do c".
Dan, I definitely like this approach. The only time I would go with the original is if there was more logic, OR I wanted to add comments (I LOVE to comment) as to why the different values were being returned.
If you want to get extra snazy... and a complete lack of readability, check this out:
<cfreturn !!findKey.recordCount />
Since a record count can never be zero, what we are really checking is zero or not-zero. As such, we can use the "!!" approach for implicit boolean conversion (an approach I just recently learned).
Oh snap, when I refreshed the page there were like 10 more comments!
@Steve, I agree with you - I would always put parenthesis into something like this.
@Ryan,
While I agree shorter is not *always* better (ever tried to debug a knarly perl script with single letter variables?!), in this case I'm with Dan (and would personally drop the comparator if I was comfortable about the possible implications with unusual values for the recordcount.
However, the fact is that "good code" depends as much on who's going to read it as any objective standards. At an extreme, feature injection, domain specific modeling and AOP are three great techniques for keeping your code DRY, but if the developers who are going to maintain it aren't familiar with them, it'll just seem like obsfucation.
The same is true of good OO code being read by coders without an OO background or good functional code for people who aren't used to functional languages.
This is a smaller example, but I think it's also a case where a lot of widely experienced programmers I know might prefer the terser form, whereas there are any developers who might find it a little too tight.
One thing I might recommend is reading books like "beautiful code". I learn a lot from reading other peoples code, and I try to focus on reading code written by the kind of developers I'd like to be . . .
I'm a big fan of this approach and I also agree with Ben and Steve... Parentheses make it just that tad bit "cleaner" in appearance. :-)
Suscribe
Follow Us
Contact
I would be interested in your experienced with CodeCop, which is an alternative version to Java or Flex's PMD tools:
http://codecop.riaforge.org/