1

Last year I posted a question here about submitting a 50 field form, and the best way to do this. That solution is still in use, and works well. However, to build these dynamic queries I'm ending up with a LOT of <cfif>'s being repeated, and I'm wondering if there is any better way to handle this. While the code ends up 'messy', the db is of course very clean due to this, and the number of writes is also kept to a minimum, but is there a better way to do the following? 

<cfif StructKeyExists(arguments.form,"data1") or StructKeyExists(arguments.form,"data2") or StructKeyExists(arguments.form,"data3")>
  <cfquery>
    insert into table1 (
      <cfif StructKeyExists(arguments.form,"data1")>data1,</cfif>
      <cfif StructKeyExists(arguments.form,"data2")>data2,</cfif>
      <cfif StructKeyExists(arguments.form,"data3")>data3,</cfif>
      userid
    )
    values (
      <cfif StructKeyExists(arguments.form,"data1")><cfqueryparam cfsqltype="cf_sql_varchar" value="#arguments.form.data1#" maxlength="30">,</cfif>
      <cfif StructKeyExists(arguments.form,"data2")><cfqueryparam cfsqltype="cf_sql_varchar" value="#arguments.form.data2#" maxlength="10">,</cfif>
      <cfif StructKeyExists(arguments.form,"data3")><cfqueryparam cfsqltype="cf_sql_varchar" value="#arguments.form.data3#" maxlength="25">,</cfif>
      <cfqueryparam cfsqltype="cf_sql_smallint" value="#arguments.form.userid#" maxlength="5">
    )
    on duplicate key update 
      <cfif StructKeyExists(arguments.form,"data1")>data1=values(data1),</cfif>
      <cfif StructKeyExists(arguments.form,"data2")>data2=values(data2),</cfif>
      <cfif StructKeyExists(arguments.form,"data3")>data3=values(data3),</cfif>
      userid=values(userid)
  </cfquery>
</cfif>

This kind of 'feels' wrong for some reason. Would it for example be smarter to rather have more writes, splitting each value into its own update, like so: 

<cfif StructKeyExists(arguments.form,"data1")>
  <cfquery>
    insert into table1 (data1,userid)
    values (<cfqueryparam cfsqltype="cf_sql_varchar" value="#arguments.form.data1#" maxlength="30">,<cfqueryparam cfsqltype="cf_sql_smallint" value="#arguments.form.userid#" maxlength="5">)
    on duplicate key update data1=values(data1),userid=values(userid)
  </cfquery>
</cfif>

etc.

Or is there a better way to do this that I am overlooking entirely?!

Community
  • 1
  • 1
sckd
  • 405
  • 3
  • 11

3 Answers3

0

If you know you have as much as 50 fields, and they're all named consistently (data1 ... dataN) I'd just have a single loop. Something like:

<cfloop index="i" from="1" to="50">
    <cfif StructKeyExists(arguments.form, "data#i#")>
        <cfquery>
            insert into table1 (data#i#,userid)
            values (<cfqueryparam cfsqltype="cf_sql_varchar" value="#arguments.form['data' & i]#" maxlength="30">,<cfqueryparam cfsqltype="cf_sql_smallint" value="#arguments.form.userid#" maxlength="5">)
            on duplicate key update data#i#=values(data#i#),userid=values(userid)
        </cfquery>
    </cfif>
</cfloop>
duncan
  • 31,401
  • 13
  • 78
  • 99
  • Sorry, I should have been clearer, but it's not quite *that* consistent. I rewrote the naming for the question, but it's more like arguments.form.departure_date / arguments.form.staffid / arguments.form.otherinfo etc. I could loop over the arguments.form I assume, but the values go into different tables (staffid goes in table 2, otherinfo goes in table 3 etc. etc.), and there's no automatic way of knowing which value goes in which table. – sckd Feb 12 '14 at 10:45
  • 2
    _"there's no automatic way of knowing"_ - so use a manual way; an array for each table with the possible fields it can receive, and loop through those. – Peter Boughton Feb 12 '14 at 11:30
  • Okay, so I loop over arguments.form, then match updated fields with their right tables using a bunch of arrays that include fields per table as well as sql type and maxlength for the cfqueryparam, and then do one insert/update per change? This would definitely keep the code nice and clean! So in terms of my original question, I end up with a bunch of single writes also in situations where one write could have included multiple values? Thanks for all the advice; definitely learning a lot here! – sckd Feb 12 '14 at 13:04
  • Just had another thought - instead of creating separate arrays/structs, you could put extra attributes on the arguments and use [getMetadata](http://cfdocs.org/getmetadata) to identify what's what - same basic idea, just might be a bit tidier that way. – Peter Boughton Feb 12 '14 at 13:19
  • Don't do one update (query) per change! You're better off combining it into one query. – Avery Martell Feb 13 '14 at 17:15
  • I like that last idea Peter. I'm going to play with some ways of dynamically writing the query in a loop like this, and see how I go. As Avery pointed out, for lots of fields, this is probably less efficient, but it does tend to primarily be an update of 2 to 5 fields, rather than all 50. – sckd Feb 15 '14 at 10:10
0

Yes there is a better way of doing it, in my opinion anyway.

First, instead of checking for the existence of all 50 fields, just check for to see if the form was submitted. Once you have done that, the only fields that might not exist are checkboxes and maybe radio buttons.

Next, do all your conditional logic before you get to a cfquery tag. Except for checkboxes and radio buttons, you are not checking for the existence of a value, you are checking for the validity. If a form field was required, make sure it's not an empty string. Also make sure that you have the proper datatypes for numeric and date fields.

During this stage, you might want to set some variables to use for the null attribute of cfqueryparam. For example, if you have an optional numeric field and the user did not fill it out, you will need a variable that equals true so that the database receives a null value. Empty strings only work with text fields.

Also keep in mind that the quality of your data is more important than the appearance of your code. While readable code is important, you are not going to process 50 form fields in 10 lines or less of code.

Dan Bracuk
  • 20,699
  • 4
  • 26
  • 43
  • 1
    I definitely already check long before this stage of the code if the form was submitted, required fields are there etc. The point is that I don't want to end up writing empty rows to tables, as that basically defeats the point of normalization (might as well have one row per person then). It's in the building up of the dynamic insert/update query that I think things must be able to be improved, at least legibility and scalability wise. – sckd Feb 12 '14 at 13:16
  • _"If a form field was required, make sure it's not an empty string."_ - empty string and no value are two different things. – Peter Boughton Feb 12 '14 at 13:17
  • *already check long before this stage of the code if the form was submitted, required fields are there* Right, but I think idea is to separate the validation (server side) from the query logic, so that by the time you run the SQL, you have already eliminated the possibility of "empty rows". Then the query code just inserts whatever it is given, since that data was already validated. Granted, sometimes you can cleanly do both - at the same time - but based on your original question, I am not sure if your table structure fits that scenario. – Leigh Feb 12 '14 at 15:17
  • I'm using the 'structkeyExists' to check if I have to run that specific query, or that specific line in the query. The validation has already happened earlier on, but this is only about building the dynamic sql queries, submitting to about 10 different tables, depending on what was edited. – sckd Feb 15 '14 at 10:03
0

I think the way you're doing it is fine.

I would NOT break up your inserts/updates into separate queries for each data field. That would result in much more overhead when a lot of fields are submitted. Imagine all 50 form fields were submitted, 50 queries, yikes.

If you need to handle NULL values, here's a sample of a way to do it within your syntax:

<cfif structKeyExists(arguments.form, "data1"><cfqueryparam value="#arguments.form.data1#" null="#(NOT len(arguments.form.data1))#" cfsqltype="cf_sql_integer">,</cfif>
Avery Martell
  • 317
  • 1
  • 7
  • I think you are right in that this comes down to a decision in terms of how many fields are likely to be submitted. From what I'm seeing, it seems to mostly be 2, 3, 4 fields, probably justifying an update to a one query per data field setup, provided that can be done automatically in a way like Peter suggests. If not, I will leave as is, as the current solution is better if lots of fields get updated at once. – sckd Feb 15 '14 at 10:08