2
\$\begingroup\$

I have a form that I submit with these variables: category_time, date_month, date_year, and status_time.

Here is an example of different form possibilities:

CATEGORY_TIME     1
DATE_MONTH    Month:
DATE_YEAR     Year:
FIELDNAMES    DATE_MONTH,DATE_YEAR,SUBMIT
STATUS_TIME   Select:
SUBMIT    submit 
----------------
CATEGORY_TIME     Choose:
DATE_MONTH    Month:
DATE_YEAR     Year:
FIELDNAMES    DATE_MONTH,DATE_YEAR,SUBMIT
STATUS_TIME   Pending
SUBMIT    submit 
----------------
CATEGORY_TIME     Choose:
DATE_MONTH    Month:
DATE_YEAR     2016
FIELDNAMES    DATE_MONTH,DATE_YEAR,SUBMIT
STATUS_TIME   Select:
SUBMIT    submit

After I submit the form depending on the variables it outputs a query. For example, with the first possibility only category_time chose, it will output the second if statement.

From what you can see, there's a lot of ifs statements that will be needed. Is there an easier way to make this work without so many ifs statement and just depending on the variables that are chosen, even if I have to use JavaScript to make it work better?

<cfif  structKeyExists(form, "category_time") ||  structKeyExists(form, "date_month")  || structKeyExists(form, "date_year") || structKeyExists(form, "status_time") >
<cfquery ....>
SELECT  *
    FROM work_timeline
    <cfif  (category_time eq 'ALL' || category_time eq 'Choose:') &&  date_month eq 'Month:' && date_year eq 'Year:' && status_time eq 'Select:' >
        order by year(date_time) desc, month(date_time) desc
    </cfif>

    <cfif  category_time neq 'ALL' && category_time neq 'Choose:' &&  date_month eq 'Month:' && date_year eq 'Year:' && status_time eq 'Select:' >
    where category_time = #category_time#
    </cfif>
    <cfif structKeyExists(form, "category_time") && date_month neq 'Month:' && date_year eq 'Year:' && category_time neq 'Choose:'
        and category_time neq 'ALL' && status eq 'Select:'>
    where category_time = #category_time# and month(date_time)=#date_month# 
    </cfif>
    more if's statments .....
</cfquery>
</cfif>
\$\endgroup\$
4
  • \$\begingroup\$ The where clause are relatively self explanatory, but I am not sure I follow how/why the filters impact the order by. Also, does the database table actually contain a record with the value "ALL", as the snippet above implies? \$\endgroup\$
    – Leigh
    Dec 4, 2015 at 19:35
  • \$\begingroup\$ @leigh the order by is because its for a timeline. The 'All' is a option in the category_time ,also 'choose' is a option. \$\endgroup\$
    – anatp_123
    Dec 4, 2015 at 22:42
  • \$\begingroup\$ What I meant was are those actual values in the database table as the sql implies, ie where category_time = 'All'? \$\endgroup\$
    – Leigh
    Dec 4, 2015 at 23:29
  • \$\begingroup\$ @leigh no it will never be 'all' in category_time , only 1,2,3,4 \$\endgroup\$
    – anatp_123
    Dec 4, 2015 at 23:53

1 Answer 1

1
\$\begingroup\$

A few things you could do to simplify the code:

  1. Rather than examining every form field to determine if the form was submitted, a single check of say - the submit button - should suffice for most purposes.

  2. Avoid using strings like "ALL" or "Choose:" for the form field values. Those are fine for display purposes, but the field values should be something more generic, like an empty string. That makes for simpler, more uniform, validation code.

    For example, if the default is an empty string, a simple LEN() check is all you need to determine if a particular field was populated. (You can also use other boolean functions like IsNumeric or IsDate, when more appropriate).

  3. Consider using cfparam to assign a default for all fields (except the submit button). That ensures the fields always exist, eliminating the need to use structKeyExists every time you access a field.

  4. With the above changes, you could also simplify the WHERE clause logic. Start with an expression that is always true. Then add additional filters based on which form fields were populated.

    NB: Using raw client supplied values in a queries puts your database at risk for sql injection. Always use cfqueryparam on all parameters. (I do not know the data types of your database columns, so modify the cfsqltype's as needed.)

    <cfparam name="FORM.category_time" default="">
    <cfparam name="FORM.date_month" default="">
    ...
    
    <cfif structKeyExists(FORM, "submit")>
    
        <cfquery ....>
            SELECT Column1, Column2, ....
            FROM   work_timeline
            WHERE  1 = 1 
    
            <!--- Filter on FORM.category_time when populated  --->
            <cfif LEN(FORM.category_time)>
                AND category_time = <cfqueryparam value="#FORM.category_time#" cfsqltype="cf_sql_integer">
            </cfif>
    
            <!--- Filter on FORM.date_month when populated --->
            <cfif LEN(FORM.date_month)>
                AND month(date_time) = <cfqueryparam value="#FORM.date_month#" cfsqltype="cf_sql_integer">
            </cfif>
    
            ... more cfif conditions
        </cfquery>
    </cfif>
    

As an aside, regarding SQL

  1. There is nothing technically wrong with using SELECT *, but in most cases it is preferable to supply a list of the columns you need. It makes the code a more readable, and also helps avoid pulling back extra data columns that are never used.

  2. Using functions on an indexed database column usually prevents the database from utilizing the index. While it may not be feasible for your current query, when possible it is best to revise expressions like this:

    WHERE month(column) = 12 AND year(column) = 2015

.. to use a more index friendly expression instead. For example:

   WHERE column >= '2015-12-01' -- Begin date at midnight       
   AND   column <  '2016-01-01' -- Day AFTER end date at midnight  
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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