Posted in Development, ColdFusion | Posted on 07-06-2006 | 5,668 views
We've all seen bad ColdFusion code. But what about really bad ColdFusion code? Let's see who can share the worse. Post your scariest experience when looking at (obviously) someone else's code. Try to keep it short and sweet and more descriptive than actual source code. As an example:
I once worked with code written by someone who had never heard of cfinclude. Instead of using the cfinclude tag to insert code into a template, they used cfhttp to perform a HTTP GET and retrieve the result - from the same server.
Just to be clear - this is not to say that ColdFusion generates bad code. Every language has some pretty scary examples written by poor or beginner developers. I'm just adding this as I know that sometimes we (as a community, including myself) can be a bit touchy, especially after seeing posts like this. (By the way, I did post a comment to that blog, but it hasn't been approved yet.)
I used to work with a product that was used by EMS dispatching agencies. Several pieces of information that are needed for all EMS calls are times (time dispatched, time enroute to call, time arrived on-scene, etc...). ALl totaled there were about 7 differnt times that needed to be tracked for each call.
How did the software stroe this information? You would think 'DATETIME' would be the perfect data type. Nope, each time had 2 coulmns, one for the date, one for the time, and each was data type 'VARCHAR'. Imagine the fun I had creating reports that displayed the average 'response time' (the time from when a unit was dispatched to when the unit arrives on scene)...it was not fun.
AAARGH!
<cfif session.user IS NOT "Admin">
<script>
alert("You are not authorized.");
history.back();
</script>
<cfabort>
</cfif>
Most develoeprs I know use this type of syntax when populating a form field.
<input type="text" name="first_name" value="#first_name#" />
Obviously this will need to be wrapped in <cfoutput> tags.
I say 'most developers' because I have actaully seen this:
<input type="text" name="first_name" value="#first_name#" />
And then at the bottom of the page, there is a block of JavaScript that handles form field population.
<script>
<cfoutput>
document.fomrs[0].first_name.value = '#first_name#';
</cfoutput>
</script>
I got tons of these....
I can top all of you :)
I "inherited" a Project where a full blown Online-Shop was written into ONE SINGLE .cfm page. Of course there was an additional Application.cfm and images. But the whole presentation and business-logic was in this monolithic file.
I think I don't have to mention that I had to rewrite it from the ground up...
Turns out for every function he was running, he would open EACH file, re-read it into memory, check to see if that was the file he wanted to work on, and then do the function. He was doing 168750 complete file reads for each 75 files he wanted to read!
<TR <CFIF #LC#/2 NEQ #INT(LC/2)#>class="table1"<cfelse>class="table2"</CFIF>>
I have worked on a cf3-4 version of a shopping cart where loads of pound signs everywhere in cfif, cfset, just making it look ugly
oh and those input's with cfoutput inside the quotes and doing this for the whole form, instead of just one cfouput around everythin
Or applications that have so much seperation between code and database, and also because no documentation, no one has any idea what the stored procedures do.
For me, it's lack of documentation of applications that really irritates me.
<cfif "#myvar.url#" EQ "ok">
<cfset var1 = "#myQuery.AA#">
<cflese>
<cfset var1 = "#myQuery.BB#">
</cfif>
Found this little gem in a enterprise level application.
Some of my other favorite things are no documentation.
<cfquery name='getDepts' datasource='ds'>
Select * from departments
</cfquery>
<cfoutput query='getDepts'>
<cfloop from = '1' to = '#len(departmentName)#' index='i'>
<cfquery name='getJobs' datasource='ds'>
SELECT * from jobs
WHERE departmentName like '#left(departmentName,i)#%'
</cfquery>
</cfloop>
</cfoutput>
There were over 100 departments returned in the original query, and then a new query was run for each department, looping through each letter in a department name!
When users would run the page that used this block of code, they would go get a cup of coffee while waiting for it to return.
I look forward to seeing a worse piece of code!
Gus
And just to be the voice of dissent ...
Joerg said: "I "inherited" a Project where a full blown Online-Shop was written into ONE SINGLE .cfm page."
That was probably one of mine. I routinely code entire apps in one CF file. And I'm going to keep on doing it. Why?
Pardon me while I rant for a sec.
Why not? The argument of presentation versus logic isn't as useful if your presentation is entirely CSS and your CF is outputting XHTML. Yeah, someone may whine about cases where you want the code to output something other than XHTML, say CSV, but that's a pretty rare case, and not really what I'm talking about. For most self-contained online apps, such as a shopping cart, copying and modifying a CSS file is hella easier than modifying a dozen dsp* files. No "where's the end of my DIV" chasing.
One of my personal pet-peeves is the shotgun approach to coding, with a zillion tiny dsp* and qry* and act* files all over the place. In the vast majority of applications that's just premature optimization. YAGNI.
Not every app needs an n-tier MVC model, just like "monolithic" does not necessarily imply "spagetti code". In fact, my monolithic apps are in many cases easier to read than the 50-file apps, as a monolithic app forces good variable names and clear logic flow. (Mine end up with a very easy validation -> logic -> retrieval -> presentation flow from top to bottom.)
Oh, and distribution doesn't get much simpler than "copy this one file into your web root". Can't even beat that with EAR/WAR files.
Each to their own, though. You keep splitting the monolithic apps into shotgun apps, and I'll keep merging the shotgun apps into monolithic ones. We'll both stay employed and happy.
But, hey, it could be worse ... Adobe could add CFGOTO to MX8.
<!--- Do a query --->
<cfquery name="foo" datasource="bar">
horrible SQL Statements
</cfquery>
<!--- Assign values to form variables --->
<cfoutput query="foo">
<cfset form.field1 = foo.field1 />
<cfset form.field2 = foo.field2 />
...
</cfoutput>
<!--- Display the form --->
<cffform ...>
<input type="text" name="field1" value="#form.field1#" />
<input type="text" name="field2" value="#form.field2#" />
...
</cfform>
Many forms had over 75 fields in them.
OH and I will personally call a Fatality on the next coder that thinks select * is uber cool.
Koder Kombat
I mean aren't we all sick of cleaning up after other people's bad code, and just want to create clean, lean, scalible apps. I know I for sure do.
The company I just started with has an old site (still on CF5) that was created back in '99. They went through and update in the last year to add some CSS styling. Unfortunately, someone didn't know much about how CSS is supposed to work.
We have a menu down he left side of the screen that displays all the main navigation items for the site. There are about 20 options. Well, those are in a table, with 3 cells each: 1 that provides some padding on th left edge, one that shows a little bullet character, and one that has the actual name of the link. Well, if you're IN that section then the text is bold and white and the bullet changes. Rather than doing this with CSS (something like add a class="active" to the current link), there are <cfif> statements around each option that decides whether or not to change its appearance if it's the current page. And it's not just a single <cfif> around that row in the table, no. There are <cfif> statements around each cell. Every. Single. One.
So, that single menu has something like 288 <cfif> statements, all so that a single option can be bold. Not only that, but the CSS styles on the page are things like "bold18ptArial" rather than "menuItem". It's killin' me. I'm updating it just on general principle!
AARRGGHH!
<cfquery name="get" datasource="#dsn#">
select *
from table
</cfquery>
<cfoutput query="get">
<cfif someBooleanColumn is "true">
<cfset #hasbeenset# = "y">
</cfif>
</cfoutput>
<cfif isdefined("hasbeenset")>
[etc]
(not sure if the "special" indentation will come through in my posting?)
This is especially good as the "get" query (nice name) has potentially 1000s of rows (including, from memory, CLOB data), and it's ONLY used for that loop to check if "someBooleanColumn" has been set. And note the nice way that even if "hasbeenset" gets set to "y" after one iteration, we better loop over the rest of the query, just in case.
I've changed the context of this code to protect the ignorant, but this is *real* code, that was in production.
The best I've seen is six "errors" (*) within ONE line of CFML. I have worked with some *special* developers.
--
Adam
(*) counting misuse of pound-signs - and that sort of thing - as an "error".
Guess they just figured our customer would never have more than 25 good ideas for shirts
<cflock name="#createUUID()#" type="exclusive" timeout="30">...[snip]</cflock>
Yep, it sets its own lock with a unique name, so if 2 people hit it at the same time, they'll both open their own uniquely named lock. I'll keep my eye out for more today :)
They did the following query and code:
SELECT * FROM myTable ORDER BY Date
#myQuery.recordCount# records available.
Not only did they return the full recordset to get a count (and they were not displaying the records anywhere), but they were nice enough to order it during the process ;)
We need a daily place to go and read/share these gems!
<cfif paramExists(session.user_name)>
<script type="javascript">
alert("You have been timed out");
location.href='logout.cfm';
</script>
</cfif>
<!-- start rest of critical business logic code -->
<cfset name = #form.name# />
<cfelse>
<cfset name = "" />
</cfif>
1. Had a frame on every page.
2. Clicking a link opened pop-ups. Every pop-up had frames.
3. Included 5-10 levels of CFINCLUDES everywhere.
4. Has an instance of 4000 pound signs in a single 1000 line template file that has nothing to do with display of data to the user.
5. Has comprehensive XSS blocking code that can be broken by mixing cases in the URL bar of any browser.
Nothing like seeing legacy spaghetti applications produced proudly by your tax dollars today!
<cfset foo = "#bar#"> DOES NOT mean you weren't a coder before CF (as was stated above).
Coldfusion is just strange sometimes. I've gotten plenty of errors on a CFC file (complex data types cannot be converted to simple datatypes or the ever popular "-1" error...) that can be fixed with a solution that seems to have no logic behind it whatsoever. But maybe it's just me... When I learned CF I was never really "taught" specifically NOT to use the hash signs (although I realize it now).
My example of bad code (while I'm here) ?
Anything using dreamweaver templates instead of just cfmodule and a layout.cfm...
I wrote this cf service monitor in cf which would cfexecute a batch file if cf failed. :rim shot:
<cfif Form.Startdate neq ''>
<CFIF Dayofyear(form.startdate) lt 10>
<cfset StartDate = "#year(form.startdate)#" & "0" & "0" &"#Dayofyear(form.startdate)#">
<CFELSEIF Dayofyear(form.startdate) lt 100 AND Dayofyear(form.startdate) gt 9>
<cfset StartDate = "#year(form.startdate)#" & "0" &"#Dayofyear(form.startdate)#">
<CFELSEIF Dayofyear(form.startdate) gt 99>
<cfset StartDate = "#year(form.startdate)#" &"#Dayofyear(form.startdate)#">
</CFIF>
</cfif>
<cfset newStructure = evaulate("#oldStructure#")>
Oh yeah... this was in a *big* loop.
One example is this:
<TR <CFIF isdefined("url.something")>BGCOLOR="##efefef"<CFELSE>BGCOLOR="###iif(myCheck MOD 2,DE('ffffff'),DE('efefef'))#"</CFIF>>
What is wrong with that? At times I'll do this:
class="rowcolor#IIF(currentrow mod 2, DE(1), DE(2))#"
How would someone with more experience write that?
The other one that I'm guilty of is the CFQUERY inside of a CFLOOP.... how can that be improved.
Thanks for any tips.
- displayed a list of listings
- displayed details for a specific listing
- displayed static pages
It determined which of the above was appropriate by getting the page from a database and determining its type (all the site pages were databased as it was connected to a slapped together CMS).
Simply splitting the single page into three pages (listings.cfm, listingdetails.cfm, staticpage.cfm) would have cleaned the code up tremendously.
As it was, page.cfm was about 1000 lines of gobbledy-gook. Of course the requisite "if url.id is 7 or url.id is 10 or url.id is 11 etc etc etc" were there in all their finery.
Use bitand() instead of MOD
I don't know about anybody else, but the nesting of greater-than and less-than signs can make it really hard to read the code. That also goes for extraneous pound signs, as others have mentioned.
What would I do with the code that you posted in question? I would put the background color logic into a separate block, like this:
<cfif structKeyExists(url,"something")>
<cfset myBgColor = "##efefef"/>
<cfelseif myCheck MOD 2 eq 1>
<cfset myBgColor = "##ffffff"/>
<cfelse>
<cfset myBgColor = "##efefef"/>
</cfif>
<tr bgcolor="#myBgColor#">
It makes it easier for me to see what the intent is (to alternate the color in some fashion), and it also helps me to see what the logic is if I need to change it in the future.
As an aside, I would probably also move the colors into a CSS file instead, and use different CSS classes for each row.
My two cents.
As far as alternating row color, my favorite solution was using a variable.
tr bgcolor="#backcolor#"
Then below usin the mod thing to switch between colors
cfif backcolor eq "e8e8e8"
cfset backcolor = "d8d8d8"
cfelse
cfset backcolor="e8e8e8"
/cfif
Sorry not exactly sure how to make code look good in a comment, but you get the basic idea.
and with this situation you don't really need a math function, unless you prefer randomness
Then you could do something like this
mybgcolors = newarray();
mybgcolor[1]="e8e8e8";
mybgcolor[2]="d8d8d8";
then in the tr
cfset pick = randrange(1,arraylen(mybgcolor))
tr bgcolor="#mybgcolor[pick]#"
so forth
My code is littered with:
<tr class="#EvenOdd[IncrementValue(CurrentRow MOD 2)]#>
Of course, somewhere before that (normally in the application.cfm), I have:
<cfset EvenOdd=ListToArray("even,odd")>
I *despise* DE().
2) Select * from view --- check
3) Realize there's duplicates of the ONE field in the view you actually need to display -- check
4) Use a CF subquery to get a distinct list -- priceless.
<option value="1" <cfif record.dayOfMonth eq "1">selected</cfif>1
<option value="2" <cfif record.dayOfMonth eq "2">selected</cfif>2
<option value="3" <cfif record.dayOfMonth eq "3">selected</cfif>3
<...etc...>
</select>
I hope this guy finally learned the subtle joys of cfloop.
<cfset WriteOutput(someVar)>
I just don't get it sometimes...
Well, of the parts that didn't turn out so good, one is memorable. You see, this guy, whoever he is, got his concept of inheritance upsidedown. It's not his fault really, because ... well. Anyway, to make a long story short, there are a bunch of CFC's that extend a parent. Each have init()'s and within each init() there's a call to super.init(passingANewInstanceOfAnotherCfcIntoTheParent).
Ok, now that it's there, we can call methods on that CFC from the child ... and we do. :) It's kind of an extravagant composition via inheritance technique that this guy invented because, well ...
<CFIF
(#ValidClaimno.mailstat# is not '') and
(#ValidClaimno.mailstat# is not ' ') and
(#ValidClaimno.mailstat# is not ' ') and
(#ValidClaimno.mailstat# is not ' ')>
I don't know where to begin...
<tr class="row#CurrentRow mod 2#">
<td>what ever</td>
</tr>
---- CSS ----
.row1 {
background-color:#F3F7F7;
}
.row0 {
background-color:#FFF;
}
<cfoutput query="qry">
<cfif active EQ 1>
#Active#
</cfif>
<cfoutput>
The recordsets were huge.
I could not believe it, and when I very politely mentioned to use the query to filter like "some" people do, he did not take it well at all.
His arguments: the many ways of skinning a cat, the reusability of the query by all the templates and the power of their Oracle DB.
I was the contractor, he was the permanent so ... you can figure out the outcome.
Come on guys, share. I am sure we all have done at least one stupid thing. :)
Call me out. I like select *, especially when you want everything. When I first started using CF, I thought it was cool. Then I thought it was lazy and made me have to search for columns that were returned.
Now I realize its just one less place I have to remember to change code when something else changes. I think it's super.
And I don't know what all the drama is about all the cfincludes. I suppose they can be abused, but you have to remember, for a while, it was the only way to have reusable cf code. What did you used to do, repeat yourself everywhere?
Open up SQL Query Analyzer and run:
SELECT * FROM [whatever table]
... and then view the execution path. Then run
SELECT [some list of columnns] FROM [whatever table]
... and view its execution path
The expense of doing using a * as opposed to explicitly naming the columns should be obvious. It isn't just about laziness or what happens to be easy for you to remember. It is truly about a *dramatic* difference in performance. If you ever write an enterprise level system where performance truly matters, using * is nothing short of writing poor code.
Sorry about that - Perhaps I should have clarified more. Of course select * (or anything else for that matter) is not <i>always</i> appropriate, but I wouldn't classify it as always inappropriate either.
If you have the need, of course you should optimize. I just don't think you should optimize before you have the need. Or, put another way, I put more value on extensibility before I put value on performance (hence, I'd rather write code that wouldn't have to change just because something else changed). It's when performance becomes an issue that I would start valuing it over the other things.
Before there were CFCs we had a "shopping cart" application where I calculated the same things in 3 different places:
1) shopping cart pre-checkout
2) secure check out
3) admin side
Now, my only excuse for 2 of those is that our secure checkout resided on a different domain than the other, and I was afraid to do such an include that would cross domains. (although now I wish I had). I have no excuse for why the other bit wasn't calculated in one spot, except I was young, dumb, and inexperienced.
To make matters worse, there are 3 different possible payment gateway accounts, that each order could go to, and it could go to one, two, or all 3 (depending on which items were being purchased). Plus in the admin side, they have the ability to place an order (with the same as above rules) and report on all the orders.
So, I guess that makes our count about 8 places where the code had to be updated any time the business rules changed (for instance, the tax rate - which was hard coded).
I was much less experienced when I wrote that, but I'm proud to report that it does all work! However, it is certainly the worst code I've ever seen.
And like most of you, I cannot stand the extraneous # signs I see everywhere.
This code ran a select * against a 60,000+ row table, looped EVERY row in the table and ran nine (yes, nine) queries against each row.
We accomplished the same in four queries in minutes. Yikes...
Or, you could do this:
<cfif queryname.recordcount gt 0>
<cfloop query="queryname" startrow="1" endrow="1">
<cfset myear = year(queryname.mydate)>
</cfloop>
</cfif>
<cfquery name="UploadTime">
Select GetDate() as time
</cfquery>
<cfset time = UploadTime.time>
<!--- and no, there is no reason that they needed the time from the db --->
Or,
<cfif there was an error uploading the file>
<cfquery>
delete the file from the db
</cfquery>
<cfif the problem was that the file was renamed>
<cfquery>
update the serverfile of the record i just deleted
</cfquery>
</cfif>
And that is just from my most current project which I have been working on for a few days
a) .mdb database (ugh!)
b) the .mdb is stored in the web root (!!!!)
c) the .mdb is not encrypted or protected in any way, and is full of 10K+ records of personal information
d) the admin page for the site is publicly accessable with a non-default page name (security through obscurity anyone? Bad!)
e) no comments. not a single one.
f) dev code and one-off fixes still sitting in the web root
wanna know the worst part? I have to figure it all out. Hundreds of templates, sometimes with filesnames that have little or nothing to do with their function, and things like file.cfm, file2.cfm, etc.
Pity me? =)
UserArray[1]
UserArray[2]
UserArray[7]
Here is another gem. I see these sort of things all over the site.
<CFOutput Query="GetBidNoBid">
<CFSet BidID=ID>
<CFSet Bid=Bid>
<CFSet Reason1=Reason1>
<CFSet Reason1_Other=Reason1_Other>
<CFSet Reason2=Reason2>
<CFSet Reason2_Other=Reason2_Other>
<CFSet Status=Status>
<CFSet DevEmpID=DevEmpID>
</cfoutput>
arguments='"+%Y-%m-%d %H:%M:%S"'
variable="get_date">
</cfexecute>
[Add Comment] [Subscribe to Comments]