HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Generating dynamic HTML

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
generatinghtmldynamic

Problem

I've made a small class whose purpose is to generate dynamic HTML. However, it seems that my implementation turned into a state machine algorithm. And so here am I seeking help on a possible design pattern that I may apply, so my code can be more readable, objected oriented and more maintainable.

```
public interface IViewContext
{
object GetContent();
}

public delegate object BindingModelToHtmlEh(object sender, BindingModelToHtmlArgs args);

public class BindingModelToHtmlArgs : EventArgs
{
public BindingModelToHtmlArgs(Type hint, Type domain)
{
this.Hint = hint;
this.Domain = domain;
}
public Type Hint { get; private set; }
public Type Domain { get; set; }
}
public class HtmlViewContext : IViewContext
{
private readonly List htmlList;
private HtmlViewContext parent;
public event BindingModelToHtmlEh ProvideValue;

protected virtual object OnProvideValue(BindingModelToHtmlArgs args)
{
BindingModelToHtmlEh handler = ProvideValue;
if (handler != null) return handler(this, args);
return null;
}

public HtmlViewContext()
{
htmlList = new List();
}

public HtmlViewContext(HtmlViewContext ctx): this()
{
parent = ctx;
}
public void AddContent(string html)
{
if (parent != null)
{
parent.AddContent(html);
return;
}
htmlList.Add(html);
}

public void AddContent(Type type, Func html)
{
if (parent != null)
{
parent.AddContent(type, html);
return;
}
htmlList.Add(new Tuple>(type, html));
}

public void AddContent(Type type, Func, string> html)
{
if (parent != null)
{
parent.AddContent(type, html);
return;
}
htmlList.Add(new Tuple, string>>(type, html));
}

public object GetContent()
{
HtmlViewResult result = new HtmlViewResult(this);

Solution

BindingModelToHtmlEh - really, an Eh suffix for a delegate?

public delegate object BindingModelToHtmlEh(object sender, BindingModelToHtmlArgs args);


Wait a minute. This looks like an event handler, it's got the sender and EventArgs-derived args parameters, but it returns an object. This is rather surprising. Events shouldn't need to return anything, they carry all the needed data in the EventArgs instance; relying on events' return value can cause nasty unsuspected bugs when/if you start having more than a single subscriber to your event.

I think you can drop that delegate and declare your event as:

public event EventHandler ProvideValue;


And then you can fix your EventArgs class to include whatever object you wanted your event handler to return. Also the naming for the event isn't terrific, I'd expect something more in line with the framework, like ValueProvided.

Aside from that, the first thing that really caught my attention was that the GetContent method you're worried about, is returning an object. I strongly believe a public API that exposes object better have a very, very, very good reason to do so.

GetContent returns the result of GetResult:

internal object GetResult(object context)
{
    currentState = context;
    it.MoveNext();
    builder.Append(it.Current);
    builder.Append(GetResult());
    return builder.ToString();
}


And that is undeniably a string. Why are you throwing away type safety like this? Any method that requires an object will be more than happy to take your string, you don't have to "downgrade" it any sooner. In fact, you should be holding on to your types for as long as it's possible to do so.

The setter for HtmlViewResult.Continues should be private. The way you have it, it's possible for a caller to mess with it, and that doesn't look like it would do any good.

I think there's a lot more to review, but that's a start :)

Code Snippets

public delegate object BindingModelToHtmlEh(object sender, BindingModelToHtmlArgs args);
public event EventHandler<BindingModelToHtmlArgs> ProvideValue;
internal object GetResult(object context)
{
    currentState = context;
    it.MoveNext();
    builder.Append(it.Current);
    builder.Append(GetResult());
    return builder.ToString();
}

Context

StackExchange Code Review Q#39827, answer score: 6

Revisions (0)

No revisions yet.