current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 0 down vote favorite

I should be able to figure this out on my own, but I can't seem to wrap my head around how to optimize this set of logic for interpreting the existence of URL parameters.

Below is my code:

<cfif ParameterExists(start) and not ParameterExists(end) and not ParameterExists(after)>
<cfquery name="STUDENT" datasource="#STUD_DB#">
    SELECT *
    FROM student_profile
    WHERE (Date_Modified BETWEEN '#token# #start#' AND '#token# 23:59:59')
</cfquery>
<cfelseif ParameterExists(end) and not ParameterExists(start) and not ParameterExists(after)>
<cfquery name="STUDENT" datasource="#STUD_DB#">
    SELECT *
    FROM student_profile
    WHERE (Date_Modified BETWEEN '#token# 00:00:00' AND '#token# #end#')
</cfquery>
<cfelseif ParameterExists(start) and ParameterExists(end) and not ParameterExists(after)>
<cfquery name="STUDENT" datasource="#STUD_DB#">
    SELECT *
    FROM student_profile
    WHERE (Date_Modified BETWEEN '#token# #start#' AND '#token# #end#')
</cfquery>
<cfelseif ParameterExists(after) and not ParameterExists(start) and not ParameterExists(end)>
<cfquery name="STUDENT" datasource="#STUD_DB#">
    SELECT *
    FROM student_profile
    WHERE (Date_Modified BETWEEN '#token# #after#' AND '#today# 23:59:59')
</cfquery>
<cfelse>
<cfquery name="STUDENT" datasource="#STUD_DB#">
    SELECT *
    FROM student_profile
    WHERE (Date_Modified BETWEEN '#token# 00:00:00' AND '#token# 23:59:59')
</cfquery>

Its a disgusting chunk of code, I know. Its operation is pretty simple though, run a query with a set of parameters if they exist.

I would like to have some sort of short circuit logic so I can just say 

<cfif (logic to check parameters) >
    <cfquery name="STUDENT" datasource="#STUD_DB#">
        SELECT *
        FROM student_profile
        WHERE (Date_Modified BETWEEN '#startdate# #starttime#' AND '#enddate# #endtime#')
    </cfquery>

This would most likely require me to set default values as well, but I am okay with that.

Any help on this?

share|improve this question

1 Answer 1

up vote 3 down vote accepted

Yeah, you're not doing yourself any favours by having the elements of the IF conditions in differing orders. It took a bit for me to re-sort everything so I could understand what the heck was going on. I think it distills down to this:

<cfscript>
    if (structKeyExists(URL, "after")){
        param name="URL.start" default=URL.after type="time";
        variables.endDate = today;  // I am not sure what scope TODAY is in, but SCOPE IT. TODAY *must* be a valid, unambiguous date string, EG YYYY-MM-DD
    }else{
        param name="URL.start" default="00:00:00" type="time";
        variables.endDate = token;  // same as with TODAY: scope it and make sure it's a valid date
    }
    param name="URL.end" default="23:59:59" type="time";

    variables.startDate = token;

    variables.start = parseDateTime("#startDate# #URL.start#"); // this could error if the contributing values don't comprise a date/time
    variables.end = parseDateTime("#endDate# #URL.end#");       // ditto
</cfscript>

<cfquery name="STUDENT" datasource="#STUD_DB#">
    SELECT *    <!--- [LIST YOUR COLUMNS HERE, DO NOT USE SELECT *] --->
    FROM student_profile
    WHERE Date_Modified BETWEEN <cfqueryparam value="#variables.start#" cfsqltype="CF_SQL_TIMESTAMP">
                        AND     <cfqueryparam value="#variables.end#"   cfsqltype="CF_SQL_TIMESTAMP">
</cfquery>

Some notes on this solution:

  1. You probably want to do better validation than how I've just used a typed param, and parseDateTime() without first checking the thing actually parses as a date time!
  2. You want to check and recheck the if statement logic I've used. I did not 100% compare it to your initial if logic

Some general notes:

  1. always scope your variables.
  2. parameterExists() has been deprecated for over ten years. Do not use it.
  3. you seldom want to SELECT *. In general, specify the columns you want; do not return columns you don't actually need
  4. NEVER hard-code values into the SQL string. ALWAYS pass them as parameters.
  5. Don't use strings in place of date/times, when you are doing date operations.
share|improve this answer
    
Great answer, I appreciate this. I am working with 10+ year old code so pardon some of the deprecated functions, this is definitely an improvement over what I had, will start using these practices in the future. Just to clarify, using sqlqueryparams help prevent sql injection if i recall? –  Jason Bristol Jun 27 '13 at 17:52
    
Protecting against SQL injection is a side effect of their purpose. Basically it enables the SQL parser / compile to separate out the SQL from the values, which means it can compile the SQL statement for later use. However it can only be reused if the SQL statement is the same. So if you include values... then it's a different compile for each value combination (if that makes sense). Also as the compiler knows what's SQL and what's value, a value containing SQL (ie: SQL injection) will still not be treated as SQL, it'll be treated as value, and not be executed. –  Adam Cameron Jun 27 '13 at 18:01
    
Oh... compilation of SQL (which needs to be done before it can run) is quite resource intensive and slow, so there's quite a performance gain from passing queries that can be compiled for later. Equally, the number of compiled queries that will be held in memory is limited, so the fewer different ones you pass to the DB, the more likely a previously compiled one will still be in memory from last time. –  Adam Cameron Jun 27 '13 at 18:03
    
Ahh this is very useful to know, thank you again for the info! –  Jason Bristol Jun 27 '13 at 19:37

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.