Generating dynamic HTML

Question

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<object> 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<object>(); 
    }

    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<object, string> html)
    {
        if (parent != null)
        {
            parent.AddContent(type, html);
            return;
        }
        htmlList.Add(new Tuple<Type, Func<object, string>>(type, html));
    }

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

    public object GetContent()
    {
        HtmlViewResult result = new HtmlViewResult(this);
        result.GetResult();
        while (result.Continues)
        {
            object value = OnProvideValue(new BindingModelToHtmlArgs(result.Hint, result.Domain));
            result.GetResult(value);
        }
        return result.GetResult();
    }    
}


private class HtmlViewResult
{
    private readonly HtmlViewContext htmlView;
    private readonly IEnumerator<object> it;
    private object currentState;
    private readonly StringBuilder builder;
    public HtmlViewResult(HtmlViewContext htmlView)
    {
        this.htmlView = htmlView;
        Continues = true;
        this.builder = new StringBuilder();
        it = GetHtmlContent();
    }

    private object GetResult()
    {
        while (it.MoveNext())
        {
            if (it.Current != null)
            {
                builder.Append(it.Current);
            }
            else
            {
                break;
            }
        }
        return builder.ToString();
    }

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

    public bool Continues
    {
        get;
        set;
    }

    public Type Hint
    {
        get; private set;
    }

    public Type Domain
    {
        get; private set;
    }

    public IEnumerator<string> GetHtmlContent()
    {
        foreach (object obj in htmlView.htmlList)
        {
            string html = obj as string;
            if (html != null)
            {
                yield return html;
            }
            else
            {
                Hint = obj.GetType();
                if (currentState != null)
                {
                    throw new InvalidOperationException("invalid state");
                }
                var f1 = obj as Tuple<Type, Func<object, string>>;
                if (f1 != null)
                {
                    Domain = f1.Item1;
                }
                var f2 = obj as Tuple<Type, Func<IEnumerable<object>, string>>;
                if (f2 != null)
                {
                    Domain = f2.Item1;
                }
                yield return null;
                if (f1 != null)
                {
                    yield return f1.Item2(currentState);
                }
                if (f2 != null)
                {
                    yield return f2.Item2(currentState as IEnumerable<object>);
                }
                currentState = null;
                Hint = null;
                Domain = null;
            }
        }
        Continues = false;
    }
}

Please read What makes a good question? I hope this code might get better review answers, if you added some introduction to the question to say what the code is supposed to do (what problem you are trying to solve). — ChrisW, Jan 23 '14 at 2:55
On my further questions I'll try to provide more context. That being said I think there is no value on editing my question since I already accepted an answer. — Bruno Costa, Jan 23 '14 at 14:35
I'd say there is value on improving the question, even now that there's an accepted answer: even if you're all set now, this thread might be useful for others and it would be better if Q&A are clear for most users. After all, isn't that what editors & reviewers do all the time here on SO/SE ? — superjos, Dec 19 '14 at 18:30
@superjos I'm on it but please remind that I am not able to update the code due to asking policy. — Bruno Costa, Dec 19 '14 at 20:35
sorry, I was not aware of this policy. Just my thoughts on keeping this place as a good source for help. — superjos, Dec 21 '14 at 11:28

ChrisWue · Accepted Answer · 2014-01-23 18:26:47Z

Apart from the comments provided by @lol.upvote:

The code as you have posted it does not compile for two reasons:

htmlList is a private member of HtmlViewContext so HtmlViewResult cannot access it. I can only assume that HtmlViewResult is actually a nested class within HtmlViewContext (oin which case it can access the private member of the outer class).
In HtmlViewContext you call result.GetContent() however GetContent() is a private method in HtmlViewResult so it cannot be accessed (be it a n inner class or not)

Now, we only review working code but because the above things are mostly access modifier problems rather than actual code bugs I'll continue with the review anyway. However you should test that your code compiles and work first before posting here.

Review:

First off: The idiomatic way to create tuples is to use Tuple.Create(). This way the compiler can infer the generic types automatically which saves you some typing work. In your case this means: Tuple.Create(type, html) - much shorter.

Your code in HtmlViewContext.GetContent() and the HtmlViewResult classes is so heavily intertwined that I'm having trouble actually figuring out what is going on there and what is supposed to happen. In the long run this is unmaintainable code. The next developer which comes along in 2 years time and tries to make a change here will just bang his head against the desk for a while until he can figure it out.

So what to do about it? Let's see if we can decipher the intend of the code:

You build a list of what I'd call content providers. Then you process that list by appending all straight html providers together until you hit a function based provider, at that point you stop, raise an event which returns an object which is the used to execute the provider returning a string (presumably dynamically generated html).

Now your problem is that you don't have a common interface for your content providers which results in a lot of type checking and casting which is a bit nasty in OO. So let's see what they have in common.

For one thing they all produce a string but while one provider can produce the string immediately others require external context. So I'd suggest something like this:

    private interface IContentProvider
    {
        BindingModelToHtmlArgs GetContextRequest(); 
        string GetContent(object context);
    }

    private class StringContentProvider : IContentProvider
    {
        private string _Content;

        public StringContentProvider(string content)
        {
            _Content = content;
        }

        public BindingModelToHtmlArgs GetContextRequest()
        {
            return null; // need no context
        }

        public string GetContent(T context)
        {
            return _Content;
        }
    }

    private class SingleObjectResolverContentProvider : IContentProvider
    {
        private Tuple<Type, Func<object, string>> _Provider;

        public SingleObjectResolverContentProvider(Tuple<Type, Func<object, string>> provider)
        {
            _Provider = provider;
        }

        public BindingModelToHtmlArgs GetContextRequest()
        {
            return new BindingModelToHtmlArgs(_Provider.GetType(), _Provider.Item1);
        }

        public string GetContent(object context)
        {
            return _Provider.Item2(context);
        }
    }

    private class ObjectCollectionResolverContentProvider : IContentProvider
    {
        private Tuple<Type, Func<IEnumerable<object>, string>> _Provider;

        public ObjectCollectionResolverContentProvider(Tuple<Type, Func<IEnumerable<object>, string>> provider)
        {
            _Provider = provider;
        }

        public BindingModelToHtmlArgs GetContextRequest()
        {
            return new BindingModelToHtmlArgs(_Provider.GetType(), _Provider.Item1);
        }

        public string GetContent(object context)
        {
            return _Provider.Item2((IEnumerable<object>)context);
        }
    }

These are nested classes inside HtmlViewContext.

You need to change your htmlList and Add methods:

    private readonly List<IContentProvider> htmlList;

    ...

    public void AddContent(string html)
    {
        if (parent != null)
        {
            parent.AddContent(html);
            return;
        }
        htmlList.Add(new StringContentProvider(html));
    }

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

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

Next thing is that you need to build the contents. The weird thing is that in your code you actually discard all the strings being returned except for the last call. I'll assume this is a bug for now.

    public string GetContent()
    {
        StringBuilder sb = new StringBuilder();

        foreach (var provider in htmlList)
        {
            object context = null;
            var contextRequest = provider.GetContextRequest();
            if (contextRequest != null)
            {
                context = OnProvideValue(contextRequest);
            }
            sb.Append(provider.GetContent(context));
        }

        return sb.ToString();
    }

And we are done. The HtmlViewRequest class is gone and the code is much more straight forward to read and extend in the future.

What I don't like is that the context is just an object - next thing I'd check is if that can't be refactored somehow to provide a stronger type.

For reference - the HtmlViewContext class now looks like this (I've renamed htmlList to reflect the intend better):

public class HtmlViewContext : IViewContext
{
    private readonly List<IContentProvider> contentProviders;
    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()
    {
        contentProviders = new List<IContentProvider>();
    }

    public HtmlViewContext(HtmlViewContext ctx)
        : this()
    {
        parent = ctx;
    }

    public void AddContent(string html)
    {
        if (parent != null)
        {
            parent.AddContent(html);
            return;
        }
        contentProviders.Add(new StringContentProvider(html));
    }

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

    public void AddContent(Type type, Func<IEnumerable<object>, string> html)
    {
        if (parent != null)
        {
            parent.AddContent(type, html);
            return;
        }
        contentProviders.Add(new ObjectCollectionResolverContentProvider(Tuple.Create(type, html));
    }

    public string GetContent()
    {
        StringBuilder sb = new StringBuilder();

        foreach (var provider in contentProviders)
        {
            object context = null;
            var contextRequest = provider.GetContextRequest();
            if (contextRequest != null)
            {
                context = OnProvideValue(contextRequest);
            }
            sb.Append(provider.GetContent(context));
        }

        return sb.ToString();
    }

    private interface IContentProvider
    {
        BindingModelToHtmlArgs GetContextRequest(); 
        string GetContent(object context);
    }

    private class StringContentProvider : IContentProvider
    {
        private string _Content;

        public StringContentProvider(string content)
        {
            _Content = content;
        }

        public BindingModelToHtmlArgs GetContextRequest()
        {
            return null; // need no context
        }

        public string GetContent(object context)
        {
            return _Content;
        }
    }

    private class SingleObjectResolverContentProvider : IContentProvider
    {
        private Tuple<Type, Func<object, string>> _Provider;

        public SingleObjectResolverContentProvider(Tuple<Type, Func<object, string>> provider)
        {
            _Provider = provider;
        }

        public BindingModelToHtmlArgs GetContextRequest()
        {
            return new BindingModelToHtmlArgs(_Provider.GetType(), _Provider.Item1);
        }

        public string GetContent(object context)
        {
            return _Provider.Item2(context);
        }
    }

    private class ObjectCollectionResolverContentProvider : IContentProvider
    {
        private Tuple<Type, Func<IEnumerable<object>, string>> _Provider;

        public ObjectCollectionResolverContentProvider(Tuple<Type, Func<IEnumerable<object>, string>> provider)
        {
            _Provider = provider;
        }

        public BindingModelToHtmlArgs GetContextRequest()
        {
            return new BindingModelToHtmlArgs(_Provider.GetType(), _Provider.Item1);
        }

        public string GetContent(object context)
        {
            return _Provider.Item2((IEnumerable<object>)context);
        }
    }
}

I just have a word for this: "fantastic!". I think it is incredible to find a pattern like that one by only viewing a not so understandable method. Thank you very much for your effort! On my further questions I'll try to provide more context as well as to make really sure that the code I provide is totally compilabe (I made some in-place changes and ended up with the visibility problem, I also forgot that HtmlViewResult was a inner class). That being said I think there is no value on editing this question since you figured out what I was doing and your answer is accepted. — Bruno Costa, Jan 23 '14 at 14:31

Mat's Mug · Answer 2 · 2014-01-23 02:43:05Z

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<BindingModelToHtmlArgs> 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 :)

You stated that events shall carry all the data needed, does that also applies for fields that the subscribers shall fill(that's why I used return)? I wasn't also expecting any cenario where that event has multiple subscribers but you got the point. My GetContent method returns object because other implementations of IViewContext for Winforms are expected and those would return a Control. — Bruno Costa, Jan 23 '14 at 3:22
Yup. Think of a CancelEventArgs: the handler sets the Cancel value to true and when the call returns to the sender, e.Cancel is acted upon - the handler doesn't return a bool. With multiple subscribers, the returned value would be that of the last handler that executed, which I believe would be the last handler that was registered with the event. I think I'd try to see what I can get out of generics before resorting to object, but you got a fair point here, a WinForms Control and a HTML string have little in common... — Mat's Mug♦, Jan 23 '14 at 3:35
Sorry I thought you were talking about GetContent but you were talking about GetResult. I believe it just slipped away because I really do care about type safety and I try to only drop it if absolutly needed and as late as possible. — Bruno Costa, Jan 23 '14 at 3:39

asked	1 year ago
viewed	248 times
active	10 months ago

current community

your communities

more stack exchange communities

Generating dynamic HTML

2 Answers 2

Your Answer

Not the answer you're looking for? Browse other questions tagged c# html or ask your own question.

Hot Network Questions

current community

your communities

more stack exchange communities

Generating dynamic HTML

2 Answers 2

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged c# html or ask your own question.

Related

Hot Network Questions