4
\$\begingroup\$

I've written a piece of code, where a pair of checkboxes and a textbox are generated N amount of times. Depending on what's in the DB, the checkbox/textbox may have a default value or are left empty.

I've pasted the code below, could anyone take a look to see if I haven't created an abomination?

<td>
    @if (singleItem != null && singleItem.FolderRead)
    {
        <input type="checkbox" id="canRead_@item.SecureFolderID" class="CHKreader" onchange="changePermissions(@item.SecureFolderID)" checked />
    }
    else
    {
        <input type="checkbox" id="canRead_@item.SecureFolderID" class="CHKreader" onchange="changePermissions(@item.SecureFolderID)" />
    }
</td>
<td>
    @if (singleItem != null && singleItem.FolderWrite)
    {
        <input type="checkbox" id="canWrite_@item.SecureFolderID" class="CHKwriter" onchange="changePermissions(@item.SecureFolderID)" checked />
    }
    else
    {
        <input type="checkbox" id="canWrite_@item.SecureFolderID" class="CHKwriter" onchange="changePermissions(@item.SecureFolderID)" />
    }
</td>

<td>
    @if (singleItem != null && singleItem.ExpireDate != null)
    {
        <input class="datepick" id="expDate-@item.SecureFolderID" onchange="changePermissions(@item.SecureFolderID)" value="@singleItem.ExpireDate.Value.ToString("yyyy-MM-dd")" />
    }
    else
    {
        <input class="datepick" id="expDate-@item.SecureFolderID" onchange="changePermissions(@item.SecureFolderID)" />
    }
</td>

I know the copy/paste code is not that clean. The textbox could be filled in with a variabele, which could remain empty if the DB record is as well. But I don't know if that makes the code more 'readable' or if such a method works for the Checkbox as well.

\$\endgroup\$
  • \$\begingroup\$ What are singleItem and item? \$\endgroup\$ – RobH May 28 '15 at 13:20
  • \$\begingroup\$ custom class objects, defined above the wall of code you see above. \$\endgroup\$ – GillesDV May 28 '15 at 14:24
  • \$\begingroup\$ So neither are loop variables? \$\endgroup\$ – RobH May 28 '15 at 14:35
  • \$\begingroup\$ Both of them are, truth be told. Both of them change every iteration \$\endgroup\$ – GillesDV May 28 '15 at 15:02
2
\$\begingroup\$

I would use the razor boolean attributes:

<td>
  <input type="checkbox" id="canRead_@item.SecureFolderID" class="CHKreader" onchange="changePermissions(@item.SecureFolderID)" checked="@(singleItem != null && singleItem.FolderRead)" />
</td>
<td>
    <input type="checkbox" id="canWrite_@item.SecureFolderID" class="CHKwriter" onchange="changePermissions(@item.SecureFolderID)" checked="@(singleItem != null && singleItem.FolderWrite)" />
</td>
<td>
    <input class="datepick" id="expDate_@item.SecureFolderID" onchange="changePermissions(@item.SecureFolderID)" value="@((singleItem != null && singleItem.ExpireDate != null)?singleItem.ExpireDate.Value.ToString("yyyy-MM-dd"):null)" />
</td>

Going further, if item/singleitem are coming from an Enumerable, there is probably a better way to generate the Ids as well. Especially if you want them to post back into a nice object. Remove the classes, remove the onchange handler.

@for(var i=0;i<Model.Count;i++)
{
  <tr data-something="@item.SecureFolderID">
    <td>
      @Html.CheckBoxFor(m=>m[i].FolderRead)
    </td>
    <td>
      @Html.CheckBoxFor(m=>m[i].FolderWrite)
    </td>
    <td>
      @Html.TextBoxFor(m=>m[i].ExpireDate,new{type="date"}) // HTML5 date w/datepicker for browsers that support it
    </td>
  </tr>
}

<style>
 #mytable td:first-child input { CHKreader styles here };
 #mytable td:nth-child(2) input { CHKwriter styles here };
 #mytable td:nth-child(3) input { datepick styles here };
/* Optionally use these instead
 input[name*="FolderRead"] { CHKreader styles here };
 input[name*="FolderWrite"] { CHKwriter styles here };
 input[name*="ExpireDate"] { datepick styles here };
*/
</style>
<script>
  $('#mytable').on("change","input",function(){
    var folderid=$(this).closest("tr").data("something");
    changePermissions(folderid);
  });
</script>

You could also use an EditorFor on the date and create your own template if you prefer that route.

\$\endgroup\$
  • \$\begingroup\$ Yes, I already changed it to something like what you just typed, though your solution is much more refined. Thank you. \$\endgroup\$ – GillesDV May 30 '15 at 13:01

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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