How NOT to Code - Challenge 12

Feb 10, 2010

Today's challenge is in ColdFusion and there are several things wrong here. See how many you can find..


<cfquery datasource="#sysDB#" name="find">
select * from users
where id='#session.id#'
</cfquery>

<cfloop query="find">
   <cfquery datasource="#sysDB#" name="findmeetings">
   select * from meetings
   where deptid='#deptid#' and status = 'open'
   </cfquery>

   ...
</cfloop>

Comments

Rudi Shumpert

Rudi Shumpert wrote on 02/10/103:41 PM

Lets see....

1) Should not use select *. List out the columns needed.

2) If the first query was written correctly, you would not need the 2nd one.

There are probably others I missed.
John Sieber

John Sieber wrote on 02/10/1010:42 PM

Replace the select * with the fields that you really want in the meetings table and then use the following query is my guess.

<cfquery datasource="#sysDB#" name="findmeetings">
SELECT *
FROM MEETINGS, USERS
WHERE USERS.ID = <cfqueryparam value="#session.id#"> AND meetings.deptid = users.deptid AND meetings.status = <cfqueryparam value="open" cf_sql_type="sql_varchar">
</cfquery>
Michael Horne

Michael Horne wrote on 02/11/106:24 AM

I'm assuming this is in a cffunction!
* cfquery result name should be local scoped
* cfquery result names are utterly meaningless. Something like local.user/local.meetings would be better.
* both queries should list field names rather than use wildcards
* both queries should use cfqueryparam
* datasources should be parameterised
* session.id should be changed to a parameter passed into the function. This is for re-use and also using session variables for testing and debugging is hellish
* the second query could be done as an outer join to the first query so the whole loop is irrelevant
* the 'and' clause in the second query should be on a new line
* the 'status' field in the second query should have [] around it to prevent errors
* deptid would be better as deptID
* the values for the deptid and status clauses in the second query should be scoped (find.deptid and find.status)

Write your comment



(it will not be displayed)