3
\$\begingroup\$

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.

\$\endgroup\$
1
  • \$\begingroup\$ Please do not modify the original code after receiving answers. \$\endgroup\$
    – Jamal
    Dec 30 '14 at 4:09
6
\$\begingroup\$

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);
\$\endgroup\$
1
  • \$\begingroup\$ First of all. Thank you very much for the valuable feedback. I'll change my code. But I was also looking for other solutions to get rid of the reflection and the dictionary. Maybe there are other ways to accomplish what I've done because I have the feeling it overkill. Any idea on that? \$\endgroup\$
    – Complexity
    May 31 '14 at 4:51

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.