Custom HtmlHelper to render a Grid

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.

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 GridFor(this HtmlHelper> htmlHelper)
 {
     return new GridBuilder(htmlHelper);
 }

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

```
public class GridBuilder : IGridBuilder
{
#region Constructors

///
/// Creates a new instance of the .
///
/// The that is used to render this one.
public GridBuilder(HtmlHelper> htmlHelper)
{
this.htmlHelper = htmlHelper;
this.properties = new Dictionary();
}

#endregion

#region Properties

public Dictionary properties;

#endregion

#region IGridBuilder Members

///
/// Gets the name of the .
///
public string name { get; private set; }

///
/// The that is used to build the grid.
///
public HtmlHelper htmlHelper { get; private set; }

///
/// Sets the name of the .
///
/// The name that the should have.
/// An that is used to construct the grid.
public IGridBuilder Name(string name)
{
this.name = name;

return this;
}

///
/// Binds an column to the grid.
///
/// The type of the column on which to bind the items.
/// The functional that will bind the control to the grid.
public void Bind(Expression> function)
{
var metadata = ModelMetadataProviders.Current.GetMetada

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 WithColumns(Action> action)
{
    action.Invoke(this);
    return this;
}

Over that:

public IGridBuilder Name(string name)
{
    this.name = name;

    return this;
}

Now the more important stuff.

#region Properties

public Dictionary 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 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 Name(string name)

Would be:

private string _name;
public string Name { get { return _name; } }
public IGridBuilder 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(Expression> function)
public IGridBuilder WithColumns(Action> 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(Expression> propertySelector)
public IGridBuilder WithColumns(Action> 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);

StackExchange Code Review Q#52125, answer score: 6