msgbartop
News, views, tips and tricks on Oracle and other fun stuff
msgbarbottom

That Fine Code

So, I was tasked with changing a simple ColdFusion application. The application was written by a former co-worker who was also a “ColdFusion developer”.

The application has an HTML form in a login.cfm page that submits to a login_action.cfm page.

Now, take a look at this login_action.cfm, what am I supposed to do? Go hang myself? The following is an exact copy/paste:

  <CFQUERY NAME="IsValidLogin" DATASOURCE="#dsn#">
  SELECT Count(*) AS LoginMatch 
  FROM users
  WHERE username = '#FORM.Login#'
  AND Password = '#FORM.Pwd#'
  </CFQUERY>

  <CFIF IsValidLogin.LoginMatch IS "0">
    <CFLOCATION URL="Loginagain.cfm">
  <CFELSEIF #FORM.Pwd# eq "change">
  <CFQUERY NAME="qryFName" DATASOURCE="#dsn#" DBTYPE="ODBC">
  SELECT first_name, username, responsibility
  From users
  Where username = '#FORM.Login#'
  </CFQUERY>    
    <CFSET Session.AdminName = #qryFName.first_name#>
    <CFSET Session.login = #qryFName.username#>
    <CFSET Session.resp = #qryFName.responsibility#>
    <CFLOCATION URL="change_password.cfm">
  <CFELSE>
  <CFQUERY NAME="qryFName" DATASOURCE="#dsn#" DBTYPE="ODBC">
  SELECT first_name, username, responsibility
  From users
  Where username = '#FORM.Login#'
  </CFQUERY>
    <CFSET Session.AdminName = #qryFName.first_name#>
    <CFSET Session.login = #qryFName.username#>
    <CFSET Session.resp = #qryFName.responsibility#>
    <CFSET Session.LoggedIn = "1">
    <CFLOCATION URL="../home.cfm">
  </cfif>

WTF!

By quickly scanning the code above, you notice that:

  • The queries do not use cfqueryparam. SQL Injection anyone?
  • Did I say queries? Why do we need three queries in the first place.
  • This “ColdFusion developer” likes to repeat not only queries, but also other blocks of code. Notice the repeated session assignments.
  • Notice the pound signs in the cfset tags. Obviously, this “ColdFusion developer” was a newcomer to ColdFusion (not to mention to database concepts) when he wrote the above code.
  • I did not list it here, but Loginagain.cfm is exactly the same as login.cfm with the exception of a “you’ve entered an invalid username or password” message.

I could not help but to re-write the above like this:

  <cfquery name="qLogin" datasource="#dsn#">
      SELECT *
      FROM users
      WHERE username = 
         <cfqueryparam cfsqltype="cf_sql_varchar" maxlength="8" value="#form.login#">
      AND Password =
         <cfqueryparam cfsqltype="cf_sql_varchar" maxlength="20" value="#form.pwd#">
  </cfquery>

  <cfif not qLogin.recordcount>
      <cflocation url="login.cfm?u=invalid" addtoken="no">
  <cfelse>
      <cfset session.AdminName = qLogin.first_name>
      <cfset session.login = qLogin.username>
      <cfset session.resp = qLogin.responsibility>
  </cfif>

  <cfif form.pwd eq "change">
      <cflocation url="change_password.cfm" addtoken="no">
  <cfelse>
      <cfset session.loggedIn = 1>
      <cflocation url="../home.cfm" addtoken="no">
  </cfif>

As I continue going through this simple application, I’m sure that I will be re-writing most of it :(

Once someone said: There is a huge difference between writing code and writing good code. I totally believe that.


Filed in ColdFusion on 17 May 06 | Tags:


Reader's Comments

  1. |

    Just a word of warning. Select * is always bad.

  2. |

    There is plenty of that kind of newbie code around I’m afraid, and lets face most of us wrote some of it when we were newbies ourselves!

  3. |

    A couple more suggestions:

    1) Don’t use SELECT *. First, it’s a readability pain for future developers because they now have to trek to the database to find out the columns returned. Second, it’s (slightly) more overhead for your RDBMS because it now has to go to the data dictionary to find out what columns are in the table since you didn’t explicitly say so.

    2) I would reverse the order of the recordCount check. For performance reasons (thought this is a slight gain, admittedly), I want the conditions that will be satisfied more frequently to be higher up my “conditional chain” so that the code doesn’t have to constantly evaluate subsequent statements.

    3) I don’t know the code, but I would venture to say that the sessions. isLoggedIn should go with the other cfsets. First, it puts all the session variables sets in one place. Second, even if they are changing their password, they technically have to be logged in, right? So set the flag before the “change” check and simply check again when the user submits their new password.

    Just a couple thoughts…

  4. |

    Thanks for the suggestions.

    Regarding the select *, I do see the point of having the columns explicitly listed instead of just the * as a way to improve readability. But what I do not see clearly is how this impacts the performance of a query.

    When you have the columns explicitly listed after the select, doesn’t the DB go to the data dictionary anyway when parsing and validating the query?

    Do you have an example that shows this performance impact when using *?

    By the way, the database I use is Oracle (10gXE as far as this application is concerned). In fact, the reason why I’m even touching this application is because we’ve decided to use Oracle 10gXE instead of MS Access.

  5. |

    some times I use SELECT * when the real query is done by a sql view. as far as your project to “fix” this code… sometimes it’s easier to just start over than fix something that bad.

  6. |

    I hear you Michael. I’m sure I will end up re-writing most of the pages.

  7. |

    I have never thought of select * or select column name being a database performance issue.

    But more of an all around performance issue by reducing the amount of information coming back to the client. Network bandwidth is always a concern in our part of a world as we have entire 100 person divisions connecting over a point to point VPN to our database servers.

    Every little bit helps. Quick example of what I mean is at Spool output from 10gXE

    Others may argue that the small number of bytes does not make a difference. Especially on a log on screen. But what if you are processing 300-400 logons an hour? Or a query that the application only processes 1 or 2 columns on a 30 or 40 column table and runs the query every time a use opens a new page… to say see if the user has any customizations to the screen? And your server processes 3000-4000 screen openings an hour.

    Those bytes add up in a hurry. But most of you who have the pleasure of gigabyte to the desktop probably will just scoff and laugh at us poor no-highspeed folks. I say this over my TOP speed dial up of 26.4KPS. proof here Dialup

    My 2 cents, might be worth less than that.

  8. |

    The good thing about being a crap coder is there is always a geek around to fix it!

    SACK DEVELOPER

  9. |

    Herod, thanks for your example. I cannot imagine myself going back to the dialup days, I would give up using the Internet if I had to use my 56K modem.

    Gurdip, there is nothing good about being crap anything. But I know what you mean.

    So, here is what I understand so far: select the columns that you need, no more, no less, performance should be a secondary issue. If you need all columns, it is OK to use select *. For better readability, try to list the column names instead of using *.

  10. |

    Eddie, I would question the reason to drag data from disk to the client if you’re not going to use it. It’s a waste of disk I/O and network bandwidth for no reason at all. Additionally, I would venture to say that the application would use less memory since it only has to store 3 columns versus all of them. This appears to be a lightweight application, so it may not impact it as much as a larger app.

  11. |

    Specifying column names instead of ‘select *’ also prevents you from having to change your data structure within the program if the table is altered (columns [re/]moved or added, etc.). Since you’re specifically naming the columns/attributes, you don’t care if they’re moved around. I also agree that you shouldn’t request more cols/atts than necessary.

  12. |

    It is best to use coldfusion if you have to develop application very quickly. The code above is fine if it does the job.

    If you want to write a good code you should move to object oriented programming like java

  13. |

    Does this mean that code written in any non OO language is not good? I do not think so. However, I do see your point.