5
\$\begingroup\$

I am developing a huge form with a lot of elements in it. The purpose of this form is editing one domain entity. Now, I finished developing the whole form and it just shows the bunch of text boxes, text areas, radio buttons and so on, allowing user to change values of entities. When user submits the form, it goes to server and server processes the data.

The form is based on Razor's HTML helpers:

(... code omitted for shortness ...)
<tr data-id="@row.Id" class="entity-row">
<td style="width: 475px">
    <span class="num">
        @Html.TextBoxFor(x => Model.Entities[i].Address, new { @class = "field field-normal", style = "text-align: right;width:475px", Id = "Address" })
    </span>
</td>
<td>
    <span class="num">
        @Html.TextBoxFor(x => Model.Entities[i].SupplyDate, "{0:dd.MM.yyyy}", new { @class = "field field-normal field-date has-datepicker", style = "text-align: right;", Id = "SupplyDate" })
    </span>
</td>
<td>
    <span class="num">
        @Html.TextBoxFor(x => Model.Entities[i].Quantity, new { @class = "field field-normal field-num", style = "text-align: right;", Id = "Quantity" })
    </span>
</td>
</tr>
(... code omitted for shortness ...)

the next request that I got is to make user can just view the data and cannot edit this. So I should make the elements on the form disabled based on some condition. I inject special property to view model indicating ability to edit data:

public class MyEntitiesViewModel
{
    (... code omitted for shortness ...)

    public Entity[] Entities { get; set; }
    public bool Editable { get; set; }

    (... code omitted for shortness ...)
}

Ok, now I would crawl over each element in the view and set disabled attribute for needed elements like this:

@{
    object elementAttributes;
    if (Model.Editable)
    {
        elementAttributes = new {@class = "field field-normal", style = "text-align: right;width:475px", Id = "Address"};
    }
    else
    {
        elementAttributes = new {@class = "field field-normal", style = "text-align: right;width:475px", Id = "Address", disabled = "disabled"};
    }
    Html.TextBoxFor(x => Model.Entities[i].Address, elementAttributes);
}

Or even better I could use IDictionary<string, object> for elementAttributes variable and get code shorter.

But I thought about introducing new helpers that accepts bool last argument indicating is element disabled (disableness of element).

Here is that I meant:

@Html.TextBoxFor(x => Model.Entities[i].Address, new { @class = "field field-normal", style = "text-align: right;width:475px", Id = "Address" }, !Model.Editable)

The last argument here !Model.Editable indicate should be or should not be the element be disabled.

Here is a set of HTML helpers I introduced to project and they are looks like working successfully:

    private static IDictionary<string, object> _GetDisabledAttributesDictionary(object attributes)
    {
        var htmlAttributesDictionary = attributes is IDictionary<string, object> ? (IDictionary<string, object>)attributes : HtmlHelper.AnonymousObjectToHtmlAttributes(attributes);
        if (htmlAttributesDictionary.ContainsKey("disabled"))
        {
            htmlAttributesDictionary.Remove("disabled");
        }
        htmlAttributesDictionary.Add("disabled", "disabled");
        return htmlAttributesDictionary;
    }

    private static MvcHtmlString _DisableHtmlElementIfNeeded<TModel, TProperty>(
        Expression<Func<TModel, TProperty>> expression, 
        object value, 
        object attributes, 
        bool disabled, 
        Func<Expression<Func<TModel, TProperty>>, object, object, MvcHtmlString> renderWithObject, 
        Func<Expression<Func<TModel, TProperty>>, object, IDictionary<string, object>, MvcHtmlString> renderWithDictionary)
    {
        if (!disabled)
            return renderWithObject(expression, value, attributes);

        var htmlAttributesDictionary = _GetDisabledAttributesDictionary(attributes);

        return renderWithDictionary(expression, value, htmlAttributesDictionary);
    }

    private static MvcHtmlString _DisableHtmlElementIfNeeded<TModel, TProperty>(
        Expression<Func<TModel, TProperty>> expression, 
        string format, 
        IDictionary<string, object> attributes, 
        bool disabled, 
        Func<Expression<Func<TModel, TProperty>>, string, IDictionary<string, object>, MvcHtmlString> renderWithDictionary)
    {
        if (!disabled)
            return renderWithDictionary(expression, format, attributes);

        var htmlAttributesDictionary = _GetDisabledAttributesDictionary(attributes);

        return renderWithDictionary(expression, format, htmlAttributesDictionary);
    }

    private static MvcHtmlString _DisableHtmlElementIfNeeded<TModel, TProperty>(
        Expression<Func<TModel, TProperty>> expression, 
        string format, 
        object attributes, 
        bool disabled, 
        Func<Expression<Func<TModel, TProperty>>, string, object, MvcHtmlString> renderWithObject, 
        Func<Expression<Func<TModel, TProperty>>, string, IDictionary<string, object>, MvcHtmlString> renderWithDictionary)
    {
        if (!disabled)
            return renderWithObject(expression, format, attributes);

        var htmlAttributesDictionary = _GetDisabledAttributesDictionary(attributes);

        return renderWithDictionary(expression, format, htmlAttributesDictionary);
    }

    private static MvcHtmlString _DisableHtmlElementIfNeeded<TModel, TProperty>(
        Expression<Func<TModel, TProperty>> expression, 
        IEnumerable<SelectListItem> selectList, 
        object attributes, 
        bool disabled, 
        Func<Expression<Func<TModel, TProperty>>, IEnumerable<SelectListItem>, object, MvcHtmlString> renderWithObject, 
        Func<Expression<Func<TModel, TProperty>>, IEnumerable<SelectListItem>, IDictionary<string, object>, MvcHtmlString> renderWithDictionary)
    {
        if (!disabled)
            return renderWithObject(expression, selectList, attributes);

        var htmlAttributesDictionary = _GetDisabledAttributesDictionary(attributes);

        return renderWithDictionary(expression, selectList, htmlAttributesDictionary);
    }

    private static MvcHtmlString _DisableHtmlElementIfNeeded<TModel, TProperty>(
        Expression<Func<TModel, TProperty>> expression, 
        object attributes, 
        bool disabled, 
        Func<Expression<Func<TModel, TProperty>>, object, MvcHtmlString> renderWithObject, 
        Func<Expression<Func<TModel, TProperty>>, IDictionary<string, object>, MvcHtmlString> renderWithDictionary)
    {
        if (!disabled)
            return renderWithObject(expression, attributes);

        var htmlAttributesDictionary = _GetDisabledAttributesDictionary(attributes);

        return renderWithDictionary(expression, htmlAttributesDictionary);
    }

    private static MvcHtmlString _DisableHtmlElementIfNeeded<TModel, TProperty>(
        Expression<Func<TModel, TProperty>> expression, 
        IDictionary<string, object> attributes, 
        bool disabled, 
        Func<Expression<Func<TModel, TProperty>>, IDictionary<string, object>, MvcHtmlString> renderWithDictionary)
    {
        if (!disabled)
            return renderWithDictionary(expression, attributes);

        var htmlAttributesDictionary = _GetDisabledAttributesDictionary(attributes);

        return renderWithDictionary(expression, htmlAttributesDictionary);
    }

    public static MvcHtmlString TextBoxFor<TModel, TProperty>(this HtmlHelper<TModel> helper, Expression<Func<TModel, TProperty>> expression, IDictionary<string, object> htmlAttributes, bool disabled)
    {
        return _DisableHtmlElementIfNeeded(
            expression,
            htmlAttributes,
            disabled,
            helper.TextBoxFor);
    }

    public static MvcHtmlString TextBoxFor<TModel, TProperty>(this HtmlHelper<TModel> helper, Expression<Func<TModel, TProperty>> expression, object htmlAttributes, bool disabled)
    {
        return _DisableHtmlElementIfNeeded(
            expression,
            htmlAttributes,
            disabled,
            helper.TextBoxFor, 
            helper.TextBoxFor);
    }

    public static MvcHtmlString TextBoxFor<TModel, TProperty>(this HtmlHelper<TModel> helper, Expression<Func<TModel, TProperty>> expression, string format, object htmlAttributes, bool disabled)
    {
        return _DisableHtmlElementIfNeeded(
            expression,
            format,
            htmlAttributes,
            disabled,
            helper.TextBoxFor,
            helper.TextBoxFor);
    }

    public static MvcHtmlString TextBoxFor<TModel, TProperty>(this HtmlHelper<TModel> helper, Expression<Func<TModel, TProperty>> expression, string format, IDictionary<string, object> htmlAttributes, bool disabled)
    {
        return _DisableHtmlElementIfNeeded(
            expression,
            format,
            htmlAttributes,
            disabled,
            helper.TextBoxFor);
    }

    public static MvcHtmlString RadioButtonFor<TModel, TProperty>(this HtmlHelper<TModel> helper, Expression<Func<TModel, TProperty>> expression, object value, object htmlAttributes, bool disabled)
    {
        return _DisableHtmlElementIfNeeded(
            expression,
            value,
            htmlAttributes,
            disabled,
            helper.RadioButtonFor,
            helper.RadioButtonFor);
    }

    public static MvcHtmlString DropDownListFor<TModel, TProperty>(this HtmlHelper<TModel> helper, Expression<Func<TModel, TProperty>> expression, IEnumerable<SelectListItem> selectList, object htmlAttributes, bool disabled)
    {
        return _DisableHtmlElementIfNeeded(
            expression,
            selectList,
            htmlAttributes,
            disabled,
            helper.DropDownListFor,
            helper.DropDownListFor);
    }

As you could see they are acting like a proxy for genuine helpers just modifying incoming sets of attributes based on condition and passes attributes to base helpers.

So, although the code works, I found it very messy and here is points of possible improvements:

  1. Repitable behavior like:

        if (!disabled)
            return renderWithObject(expression, format, attributes);
    
        var htmlAttributesDictionary = _GetDisabledAttributesDictionary(attributes);
    
        return renderWithDictionary(expression, format, htmlAttributesDictionary);
    

The algorithm here is simple:

  • if disabled then render with helper that accepts attributes in form of object
  • otherwise make sure the attributes have disabled attribute and render with helper that accepts attributes in form of dictionary.

    1. Configuration of base helpers in this lines:

      return _DisableHtmlElementIfNeeded(
      expression,
      selectList,
      htmlAttributes,
      disabled,
      helper.DropDownListFor,
      helper.DropDownListFor);
      

is there any way to map helpers more elegantly? I thought of using attributes or any kind of Map methods in a way mappers configured.

So any other recommendations and advices are very welcomed!

\$\endgroup\$
  • \$\begingroup\$ Good first question, welcome on CR :) \$\endgroup\$ – IEatBagels Oct 23 '15 at 13:20
  • \$\begingroup\$ @TopinFrassi Thank you! I think I will use this site often, good work. \$\endgroup\$ – kseen Oct 24 '15 at 14:07
2
+50
\$\begingroup\$

Focusing on the helper methods only.

Lets start with the naming of the private static methods to get this out of our way.

Method names starting with an underscore is IMO a no go. Based on the NET naming guidelines one shouldn't use any underscores in the names. One exception though is the use of a prefixing underscore for class level variables.
It just looks weird if you call call such methods.


private static IDictionary<string, object> _GetDisabledAttributesDictionary(object attributes)
{
    var htmlAttributesDictionary = attributes is IDictionary<string, object> ? (IDictionary<string, object>)attributes : HtmlHelper.AnonymousObjectToHtmlAttributes(attributes);
    if (htmlAttributesDictionary.ContainsKey("disabled"))
    {
        htmlAttributesDictionary.Remove("disabled");
    }
    htmlAttributesDictionary.Add("disabled", "disabled");
    return htmlAttributesDictionary;
}

There is no need to use ContainsKey() before you use Remove() because if there is no entry with the passed key the method will just return without throwing an exception. Because you add the same key later, you could simplify this by just using the setter of the Index property like so

private static IDictionary<string, object> _GetDisabledAttributesDictionary(object attributes)
{
    var htmlAttributesDictionary = attributes is IDictionary<string, object> ? (IDictionary<string, object>)attributes : HtmlHelper.AnonymousObjectToHtmlAttributes(attributes);

    htmlAttributesDictionary["disabled"] = "disabled";
    return htmlAttributesDictionary;
}

If the key "disabled" isn't in the dictionary it will be added with the value "disabled" otherwise the value will be overwritten.

This ternary

var htmlAttributesDictionary = attributes is IDictionary<string, object> ? (IDictionary<string, object>)attributes : htmlHelper.AnonymousObjectToHtmlAttributes(attributes);

is somewhat ugly and less performant. First you are using is and if this evaluates to true you do the cast. A faster way would be to use the soft casting as and use the null-coalescing operator ?? which leads to

private static IDictionary<string, object> _GetDisabledAttributesDictionary(object attributes)
{
    var htmlAttributesDictionary = attributes as IDictionary<string, object> ?? HtmlHelper.AnonymousObjectToHtmlAttributes(attributes);

    htmlAttributesDictionary["disabled"] = "disabled";
    return htmlAttributesDictionary;
}

In all of the remaining private static methods you have this

if (!disabled)
    return renderWithObject(expression, value, attributes);

which doesn't meet the style used before where you have used braces {} which is recommended. Using braces makes your code less error prone, so do it always.

Whether you decide to use them or not, you should stick to the chosen style. Mixing styles makes it harder to read the code.

\$\endgroup\$
  • \$\begingroup\$ Very useful answer! Thank you! Will fix all you mentioned! \$\endgroup\$ – kseen Oct 29 '15 at 15:06
5
\$\begingroup\$

With the danger of not understanding your question entirely as it stands now, you have MyEntitiesViewModel with a enabled field. Why don't you use to trigger a CSS class for the entire entity which disables that entity?

Why do you reiterate over the attributes, and create loads of helpers which I don't understand why you need. I'm not entirely sure about the syntax just now, but wouldn't something like the following do the trick:

<tr data-id="@row.Id" class="entity-row  @(@Model.Editable ? "enabled" : "disabled")"">

    <td style="width: 475px">
        <span class="num">
            @Html.TextBoxFor(x => Model.Entities[i].Address, new { @class = "field field-normal", style = "text-align: right;width:475px", Id = "Address" })
        </span>
    </td>
</tr>

Variant over the same theme

If you don't want to use logic in the class you could also add a EnabledClass property to your viewmodel, using something like the following:

public string EnabledClass {
   get { return Editable ? "enabled" : "disabled" } 
}

And corresponding in the <tr> definition:

and then your view will be simpler and will contain no logic at all:

<tr data-id="@row.Id" class="entity-row  @Model.EnabledClass">
\$\endgroup\$
  • \$\begingroup\$ Thank you for your response! The reason why I didn't use CSS class it is because of the CSS class and disabled attribute are for different purposes! CSS class is for managing the way element is looks in user interface. You can't disable element like disabled attribute does. The attribute prevents the user interaction with element and even make element not sending to the server on form submission. And it is also takes care of how this element should be viewed in page. \$\endgroup\$ – kseen Oct 24 '15 at 14:14
  • \$\begingroup\$ @kseen, you can achieve much of the same using css, or use similar trick to set the attribute. A few years back, the firm I worked for used code similar to this to disable stuff in production code. \$\endgroup\$ – holroy Oct 24 '15 at 14:36
  • \$\begingroup\$ I think it is opposite to the nature of HTML and to achieve using this way we need to spend too much efforts.I think using embedded disabled is much easier. \$\endgroup\$ – kseen Oct 24 '15 at 17:08

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.