Custom HtmlHelper to render a Grid

Question

I have created a custom HtmlHelper to render a Grid, and it's working fine, but I know for sure that it isn't written really nicely.

I'll provide you with the code that's most important for the grid.

First of all, the model which I pass to my grid is an IEnumerable<T>.

Then, the Razor syntax for the grid is as follows:

@(Html.GridFor()
    .Name("PageOverviewGrid")
    .WithColumns(model =>
    {
        model.Bind(x => x.Name);
        model.Bind(x => x.DateCreated);
        model.Bind(x => x.DateUpdated);
    })
)

Then I do have a custom HtmlHelper to render this grid:

 public static IGridBuilder<TModel> GridFor<TModel>(this HtmlHelper<IEnumerable<TModel>> htmlHelper)
 {
     return new GridBuilder<TModel>(htmlHelper);
 }

Then the GridBuilder class itself (it's constructed with interfaces so I can use a fluent API):

public class GridBuilder<TModel> : IGridBuilder<TModel>
{
    #region Constructors

    /// <summary>
    /// Creates a new instance of the <see cref="GridBuilder{TModel}"/>.
    /// </summary>
    /// <param name="htmlHelper">The <see cref="HtmlHelper{TModel}"/> that is used to render this one.</param>
    public GridBuilder(HtmlHelper<IEnumerable<TModel>> htmlHelper)
    {
        this.htmlHelper = htmlHelper;
        this.properties = new Dictionary<string, string>();
    }

    #endregion

    #region Properties

    public Dictionary<string, string> properties;

    #endregion

    #region IGridBuilder Members

    /// <summary>
    /// Gets the name of the <see cref="IGridBuilder{TModel}"/>.
    /// </summary>
    public string name { get; private set; }

    /// <summary>
    /// The <see cref="HtmlHelper"/> that is used to build the grid.
    /// </summary>
    public HtmlHelper htmlHelper { get; private set; }

    /// <summary>
    /// Sets the name of the <see cref="IGridBuilder{TModel}"/>.
    /// </summary>
    /// <param name="name">The name that the <see cref="IGridBuilder{TModel}"/> should have.</param>
    /// <returns>An <see cref="IGridBuilder{TModel}"/> that is used to construct the grid.</returns>
    public IGridBuilder<TModel> Name(string name)
    {
        this.name = name;

        return this;
    }

    /// <summary>
    /// Binds an column to the grid.
    /// </summary>
    /// <typeparam name="TItem">The type of the column on which to bind the items.</typeparam>
    /// <param name="function">The functional that will bind the control to the grid.</param>
    public void Bind<TItem>(Expression<Func<TModel, TItem>> function)
    {
        var metadata = ModelMetadataProviders.Current.GetMetadataForProperty(() => Activator.CreateInstance<TModel>(), typeof(TModel), name);

        properties.Add(metadata.PropertyName, metadata.DisplayName ?? metadata.PropertyName);
    }

    /// <summary>
    /// Set the columns of the model that should be bound to grid.
    /// </summary>
    /// <param name="action">The action that will bind all the columns.</param>
    /// <returns>An <see cref="IGridBuilder{TModel}"/> that is used to construct the grid.</returns>
    public IGridBuilder<TModel> WithColumns(Action<IGridBuilder<TModel>> action)
    {
        action.Invoke(this);
        return this;
    }

    #endregion

    #region IHtmlString Members

    /// <summary>
    /// Returns an HTML-encoded string.
    /// </summary>
    /// <returns>Returns an HTML-encoded string.</returns>
    public string ToHtmlString()
    {
        // Additional rendering is done here.
    }
}

And then now the things about which I'm concerned:

Retrieving the property display name or property name. (It's done through reflection right now, but I'm thinking that there is more friendly solution).

How can I render the data of the entity. For example, I want to have the DateCreated of a single entity in the grid.

foreach (var entity in htmlHelper.ViewData.Model as IEnumerable<GridPageFolderViewModel>)
{
    // For the entity get the properties and look in the directory of properties if it should be displayed. If that's the case, render it.
}

I have the feeling that this could be optimized, but it's my very first HTML extension. I would appreciate any help.

Answer 1

I like the fluent API. Nitpicks first:

• Drop the #region blocks. They only add clutter, for example #region IHtmlString Members is only confusing, since it's not immediately apparent that GridBuilder implements IHtmlString (perhaps IGridBuilder does? I'm not super familiar with these extensions myself, but I don't think it matters - comments shouldn't be misleading or confusing, period). Also #region Properties is a big fat lie; I'll get back to that.

• Vertical spacing isn't consistent. I prefer this:

  public IGridBuilder<TModel> WithColumns(Action<IGridBuilder<TModel>> action)
  {
      action.Invoke(this);
      return this;
  }
  

  Over that:

  public IGridBuilder<TModel> Name(string name)
  {
      this.name = name;
  
      return this;
  }
  

---

Now the more important stuff.

#region Properties

public Dictionary<string, string> properties;

#endregion

This isn't a property, the comment describing the region is very, very misleading. This is a publicly exposed field - client code is completely free to reassign the Dictionary<string, string> reference to whatever it wants. Exposing public fields breaks encapsulations. Expose properties, not fields.

Your naming conventions are confusing, inconsistent and error-prone. All public members should be PascalCase - keep camelCase for local variables and private fields; I'd also recommend prefixing private fields with an _ underscore (still _camelCase for private fields) so that you can get rid of the this qualifier, and only use it when it's actually needed - i.e. when you're returning this, or passing this as a parameter to a method/delegate.

public string name { get; private set; }
public IGridBuilder<TModel> Name(string name)

Would be:

private string _name;
public string Name { get { return _name; } }
public IGridBuilder<TModel> WithName(string name);

Notice how none of these names are ambiguous in any way.

---

The parameter naming could be more meaningful here:

public void Bind<TItem>(Expression<Func<TModel, TItem>> function)
public IGridBuilder<TModel> WithColumns(Action<IGridBuilder<TModel>> action)

These parameters are closer to being methods, and naming them as such (still camelCase though) would be more helpful and wouldn't require relying on the XML documentation / IntelliSense to know what that function/action is supposed to be doing:

public void Bind<TItem>(Expression<Func<TModel, TItem>> propertySelector)
public IGridBuilder<TModel> WithColumns(Action<IGridBuilder<TModel>> bindAllColumns)

I'm recommending such naming, because this:

action.Invoke(this);

Could then also be written like this:

bindAllColumns(this);

..which is much more meaningful.

---

Lastly, this line may have a minor bug:

properties.Add(metadata.PropertyName, metadata.DisplayName ?? metadata.PropertyName);

DisplayName being a string, the null-coalescing operator isn't ideal. I'd split it in two instructions:

var displayName = string.IsNullOrEmpty(metadata.DisplayName)
                            ? metadata.PropertyName
                            : metadata.DisplayName;
properties.Add(metadata.PropertyName, displayName);