ColdFusion processing a form more efficiently and a minor error

Question

I have a form processing that I am sure can be done more efficiently and there is an error in the result set although not "life threatening" just not correct.

The purpose of the page is to associate an item with a program and at the same time associate a designation within the program -- B and F fields are checkboxes allowing the item to be associated with multiple programs and designations within that specific program. (edited for clarity) Example:

Item: Lightsaber

• Program: Jedi Training
• Designation: Tool(b) (yes)

• Designation: Weapon(f) (no)

• Program: Jedi Master

• Designation: Tool(b) (yes)

• Designation: Weapon(f) (yes)

• Program: Smuggler

• Designation: Tool(b) (no)
• Designation: Weapon(f) (no)

---

Form:

 <form action="#CGI.SCRIPT_NAME#" method="post" name="program">
    <label>Item</label>
    <select id="item" name="item">
    <!---//loop through and display items --->
    <cfloop query="getitems">
        <option value="#itemid#">#itemname#</option>
    </cfloop>
    </select>   
    <table>
    <!---//loop through and display programs --->
    <cfloop query="getprogram">
    <tr>
    <td>#programname#</td>
    <td><input type="checkbox" id="B#programid#" name="B#programid#"></td>
    <td><input type="checkbox" id="F#programid#" name="F#programid#"></td>
    </tr>
    </cfloop>
    </table>
    <input type="submit">
    </form>

Action Page:

<!---// is there is a form being processed --->
<cfif #CGI.REQUEST_METHOD# is 'post'>
<!---// create program list from query --->
 <cfset pl = ValueList(query.var, ','>
<!---// set addtl form var's --->
 <cfset listassid = 'form.item'>
<!---// loop over program list ---> 
 <cfloop list="#pl#" index="i">
<!---// loop over form fields --->
  <cfloop list="form.fieldnames" index="field">
   <cfif #field# EQ 'B'&#i#>
<!---// if field is B and var, set designation true --->     
    <cfset b = 1>
   <cfelse>
<!---// it's not, set to null --->
     <cfset b = 'null'>
   </cfif>
   <cfif #field# EQ 'F'&#i#>
<!---// if field is F and var, set designation true --->  
     <cfset f = 1>
   <cfelse>
<!---// it's not, set to null --->
     <cfset f = 'null'>
   </cfif>
   <cfif b EQ 'null' AND f EQ 'null'>
<!---// if both are null then skip --->   
    //do nothing
   <cfelse>
<!---//insert record into table --->
     insert into table (table fields)
     (#i#, #listassid#, #b#, #f# )
   </cfif>
  </cfloop>
 </cfloop> 
</cfif>

The result should be:

id  item_id program_id  B   F
1   24      1           x   
2   32      2           x   x

The actual result is:

id  item_id program_id  B   F
1   24      1           x   
2   32      2           x   
3   32      2           x

Thank you in advance for any clarification and efficiencies you can suggest.

Answer 1

I think a slightly different naming convention would help resolve the current issue, and improve the readability of the code.

One option is to store all of the project id's in a hidden form field. Be sure to use the same "name", so the id's are submitted as a CSV list. Also, I would recommend more meaningful names for the checkboxes, to improve readability. For example, "isSomething" rather than just "B" or "F":

<select id="item" name="itemID">
    <cfoutput query="getitems">
        <option value="#itemid#">#itemname#</option>
    </cfoutput>
</select>   
...
<cfoutput query="getPrograms">
    <input type="hidden" name="programIDList" value="#programID#">
    ...
    <!--- Set checkbox values to 1 / Yes --->
    <td><input type="checkbox" name="isTool_#programID#" value="1"></td>
    <td><input type="checkbox" name="isWeapon_#programID#" value="1"></td>
    ...
</cfoutput>

When the form is submitted, loop through the list of project id's and use the current id to extract the values of each set of checkboxes. If either box was checked, insert a record into the database.

_{* Note: Checkboxes are only submitted when "checked". Before accessing the field, either use structKeyExists to verify the field exists OR use cfparam to set a default value for the field.}

<!--- Loop through list of available program id's --->
<cfloop list="#form.programIDList#" index="variables.programID">

    <!--- Ensure fields exist. Set default value  = 0 / No --->
    <cfparam name="form.isTool_#variables.programID#" default="0">
    <cfparam name="form.isWeapon_#variables.programID#" default="0">

    <!--- Extract the current values --->
    <cfset variables.isTool     = FORM["isTool_"& variables.programID ] >
    <cfset variables.isWeapon   = FORM["isWeapon_"& variables.programID ] >

    <!--- DEBUG ONLY: Display current values --->
    <cfoutput>
        <hr>variables.programID = #variables.programID#
        <br>variables.isTool = #variables.isTool#
        <br>variables.isWeapon = #variables.isWeapon#
        <br>form.itemID = #form.itemID#
    </cfoutput>


    <!--- If either box was checked, save to database --->
    <cfif variables.isTool || variables.isWeapon>
        ... run cfquery here 
    </cfif>

</cfloop>

Two important notes about database queries:

• Never use raw client values in SQL. Instead use cfqueryparam. It helps protects your database against sql injection. It also improves query performance when a statement is executed multiple times (as in this case)

• When executing multiple (related) queries, be sure to wrap them in a cftransaction to ensure consistency, ie All statements succeed or fail as a unit.