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

Functional Html builder

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

Problem

This is my third attempt to create a easy to use html builder because I wasn't really satisfied with the first one that wasn't extendable at all and even the one using dynamics wasn't much better. It was difficult to use due to everything being dynamic and had to be casted everywhere but what's worse it was hard to find the bug if something went wrong.

This time I tried to do it the functional way. You validate how well I did or not.

Based on your suggestions I now use an interface for the base definition:

public interface IMarkupElement : ICollection
{
    string Name { get; }
    IDictionary Attributes { get; }
    IMarkupElement Parent { get; set; }
    int Depth { get; }
}


and derive two types from it. The actual markup-element

```
[DebuggerDisplay("{DebuggerDisplay,nq}")]
public class MarkupElement : IMarkupElement
{
private readonly List _content = new List();

public MarkupElement(string name, IEnumerable content)
{
Name = name.NonEmptyOrNull() ?? throw new ArgumentNullException(nameof(name));
Add(content ?? throw new ArgumentNullException(nameof(content)));
Attributes = new Dictionary(StringComparer.OrdinalIgnoreCase);
}

private string DebuggerDisplay => $"";

#region IMarkupElement

public static IMarkupElement Builder => new NullMarkupElement();
public string Name { get; }
public IDictionary Attributes { get; }
public IMarkupElement Parent { get; set; }

public int Depth
{
get
{
var parent = Parent;
var depth = parent != null ? 1 : 0;
while ((parent = parent?.Parent) != null) depth++;
return depth;
}
}

#endregion

#region ICollection

public int Count => _content.Count;
public bool IsReadOnly => false;
public void Add(object item)
{
if (item == null) return;

switch (item)
{
case IMarkupElement e: e.Parent = this; break;
case

Solution

NonEmptyOrNull()


the name is ambiguous. Is it !empty || null or is it !(empty or null)? Easy to get it wrong. I would just use regular !String.IsNullOrEmpty() - no ambiguity here.

content ?? throw new ArgumentNullException(nameof(content))


why content can not be null? I would assume that at some point you are going to add an element with no children/content. Should I pass new IMarkupElement[0] in that case? I would rather just pass null (or pass nothing at all).

public int Depth
{
    get
    {
        var parent = Parent;
        var depth = parent != null ? 1 : 0;
        while ((parent = parent?.Parent) != null) depth++;
        return depth;
    }
}


Can't you just return Parent == null ? 0 : Parent.Depth + 1?

Add(object item)


IMHO, this method's implemntation has big wtfsurprise factor. Imagine if ArrayList were to silently drop some of the items you've added based on some undocumented criteria. I suggest you:

  • Use Add(object) only to add single object to the collection, because that's the contract defined by ICollection. Use different AddRange method to add multiple items.



  • throw if invalid item is added. Otherwise add the item to collection.



  • Document any weird or unexpected behavior. You might have a reason to implement Add(object) that way, but this reason is not obvious.



NullMarkupElement should not have public constructor since it is only used in specific place with specific purpose. You can also consider just straight up using ((IMarkupElement)null) as your "builder".

Overall I think this implementation would be easier to understand/maintain if you were to "sanitize" during html construction, and not during rendering. Instead of having special rules on how to render non-IMarkupElement objects, you should have special rules on how to convert those object to IMarkupElement. This way you should be able to work with strongly typed IMarkupElement : ICollection instead of IMarkupElement : ICollection.

Code Snippets

NonEmptyOrNull()
content ?? throw new ArgumentNullException(nameof(content))
public int Depth
{
    get
    {
        var parent = Parent;
        var depth = parent != null ? 1 : 0;
        while ((parent = parent?.Parent) != null) depth++;
        return depth;
    }
}
Add(object item)

Context

StackExchange Code Review Q#159043, answer score: 3

Revisions (0)

No revisions yet.