Daily WTF - Check Out This Gem
Posted By : todd sharp Posted At : October 9, 2008 4:09 PM Posted In: ColdFusion
16
A friend was searching through some legacy code when he found this absolute beauty (names changed to protect the guilty):
<cfif NOT IsNumeric(productID) or len(productID) gt 10>
<cfthrow message="Invalid Number!" type="NotNumber">
</cfif>
<cftry>
<cfcatch Type="NotNumber">
</cfcatch>
</cftry>
<cfthrow message="Invalid Number!" type="NotNumber">
</cfif>
<cftry>
<cfcatch Type="NotNumber">
</cfcatch>
</cftry>
Wow where do I begin explaining whats wrong with this code? Actually I think it speaks for itself.
So nice.
First of all, I have a problem with the fact that the code checks the len of the productID and throws a custom exception if it is not numeric or the length is greater then 10. Since when is an number not a number if it is greater then 10 digits?
Secondly, the whole try catch block is a giant waste. I imagine they probably wanted to put the cfif statement inside of the try/catch block - but even so the cfcatch isn't doing anything. No logging, no email, no nothing!! Even if the code were written correctly, I find it to be a bad practice to throw a custom exception and immediately catch it within a try/catch block...what's the point? I would venture to guess what they really were trying to do is simply validate the productID - if so then I'd get rid of the throw/try/catch and just validate per your business rules and create a friendly error message to show the user if the validation fails.
Check this out.
This:
http://mgt.pastebin.com/f7fb1e536
Renders this
http://www.myinternetisbroken.com/files/Shite_OutP...
What a load of crap that is!!
actually that code isn't that bad at all. pretty readable from the code itself and it's commented nicely.
the thing is, when was this code written? if it was written back in the 4.0 days, you didn't have a lot of the functionality that we do today. functions, QoQ and a lot of things didn't exists, but arrays and structures did.
I'm only stating this because before your laugh and comment on what someone wrote, you need to know the whole story.
So if this was written back in the 4.0 days, put this guy on the back. If it was written 2 weeks ago under Cf8, kick his ass.
Wow, I don't think in all my years I've ever seen such a fundamental lack of understanding the use of a query. At first I assumed that this was stateless, with no DB in the back or something ... until I saw the CFQUERY and the third set of CFSET tags. Ouch
-jfish
This was done just before I came on... V.7 so it was a couple of years ago. This guy was a piece of work....
I ranted about here.
http://www.myinternetisbroken.com/index.cfm/2007/4...-
Thank good I don't do all that maintenance stuff but when I do, it makes Baby Jesus cry.
after reading your post and comment i must recant my previous position.
IT YOUR DUTY TO FIND THIS PERSON AND DESTROY HIM!!!
I only wish you can give us his name so I may warn my HR department about ever hiring him.
Surely there would have at least been something in the catch section, not much point going to the trouble to throw and catch a custom exception and then do nothing.
Not only that, but since nothing is in the try block nothing is going to be caught.
Kind regards
</cliff>
Might want to also keep an eye on dan vega's blog I think he's going to start posting some similar examples.