How NOT to Code - Example 1

Jan 09, 2010

I recently gave a presentation to our local ColdFusion group titled 'How NOT to Code in ColdFusion' where I showed simple mistakes that I run across. I figured this might make an interesting series of posts on my blog as well. So here's the first example. The game is to find as many mistakes with the code as possible, and yes, there will always be several to pull out of them. Naturally you may want to see how many you can find before scrolling down for the comments where people give their answers. As far as style type issues I'm not as concern with them per se, but you're free to list them if you like. I'll probably bounce around with languages like AS3, Flex, CFML and AJAX - so hopefully people will find this fun.

 


<cfquery datasource="#sysdb#" name="adduser">
insert into users(firstname,mi,lastname,email)
values('#firstname#','#mi#','#lastname#','#email#');
</cfquery>

<cfquery datasource="#sysdb#" name="id">
select max(id) as newid from users where 0=0
</cfquery>

<cfset session.id = id.newid/>

 

Comments

Nathan Strutz

Nathan Strutz wrote on 01/10/10 4:21 PM

Fun! A game!

Let's see. First query doesn't need a name attribute. Values need cfqueryparam tags instead of raw outputs. Semicolon is unnecessary and not consistent with the second query.

Depending on your database, the 2nd query could be put inside the same cfquery block, reducing the number of round trips (but then putting the need for the name attribute back in). Selecting the max id is a crappy way to get the ID of the row of data that was just inserted - there are typically database specific ways to do this that will not choke on race conditions such as SQLServer's SCOPE_IDENTITY(), or in Oracle where you need to create the identity before you insert the record.

If you're going to have 2 cfqueries that are related, they should be nested in a cftransaction. To avoid race conditions with this exact code, you may also need to put on a cflock around the entire block. Neither of these would be necessary if they were combined into one cfquery statement.

Hmm... think I missed anything?
Dave Ferguson

Dave Ferguson wrote on 01/11/10 10:43 AM

Oh... where do I begin... lets see...

The first and I think the biggest issue with this is the 2nd sql statement to get the ID. Since both sql statements are not wrapped in a transaction it is possible that the ID entered is not the one that was just inserted. This could be overcome by adding a cftransaction or by adding some of the insert data (lastname, email) to the where clause when getting the id.

With ColdFusion 8 and greater, you can access the result struct and get the id of the row inserted. This is not supported by all databases so your millage may vary.

With the first query the name attribute can be omitted since there is no sql return. Also, the query should be using cfqueryparam for the insert values.


There is probably more but nothing is jumping out at me right now.

--Dave
John Mason

John Mason wrote on 01/13/10 1:16 PM

All good. Yep, the point here was this is a really bad way to get a new id. The lack of queryparam and transactions makes this even worse. The other possible solution is to use createUUID() first and then insert that into the table as the row id. You can then use it as needed on the cf side of things.
Naveen

Naveen wrote on 05/17/10 9:06 AM

What is the purpose of using "where 0 = 0" in the select clause?
John Mason

John Mason wrote on 05/17/10 2:16 PM

@Naveen,

It's superfluous. Just old school sql you will run across from time to time. It basically means report everything if 0=0 which will always be true.

Write your comment



(it will not be displayed)