Elegant approach to building unordered list from XML

Question

How can I improve on this rather ugly method that builds an HTML unordered list from an XML file?

I maintain a collection of ASP.NET webforms that all need to pull in the same site navigation as our main website. The forms have a master page that mimics the main site template, and we want any changes to the main navigation to flow through to the forms automatically.

The main site uses an XML document to generate the navigation, and the document uses inconsistent formatting for the links (some have "http://mysite.com" hardcoded, and some are relative to the main site "/subsite").

Here's what I have right now:

private void LoadNavigation()
{
    string urlPrefix = "http://mysite.com";
    string xmlFilePath = ConfigurationManager.AppSettings["NavigationUrl"].ToString();
    XmlDocument doc = new XmlDocument();
    doc.Load(xmlFilePath);
    XmlNodeList navigationItems = doc.DocumentElement.FirstChild.ChildNodes;

    foreach (XmlNode item in navigationItems)
    {
        if (item.Attributes.GetNamedItem("url").Value.Contains("http://")) // link is hard coded
        {
            navList.InnerHtml += "<li> <a href=" + '"' + item.Attributes.GetNamedItem("url").Value
                + '"' + ">" + item.Attributes.GetNamedItem("title").Value + "</a>|</li>";
        }

        else
        {
            navList.InnerHtml += "<li> <a href=" + '"' + urlPrefix + item.Attributes.GetNamedItem("url").Value
                + '"' + ">" + item.Attributes.GetNamedItem("title").Value; //relative link

            if (item.NextSibling == null)
            {
                navList.InnerHtml += "</a></li>"; //last item in list
            }
            else
            {
                navList.InnerHtml += "</a>|</li>"; //not the last item in list
            }
        }
    }
}

Believe it or not, this is an improvement over the original, which included a hardcoded URL for the XML file and got the values out of the XML document like this:

XmlNodeList nodeList = doc.ChildNodes[1].ChildNodes[0].ChildNodes;

But it still needs a lot of work. Please have at it.

Answer 1

The part that looks most problematic in your code is the if statement: the code in the if part and the else part is almost the same except that you prepend urlPrefix to the URL if it is relative. (Also you're only checking whether the node is the last if the URL is relative, which does not seem right). At the very least I'd factor this out into a helper method, which takes the absolute URL as a parameter to remove the code duplication.

However there's an even better way: .net already comes with the System.Uri class, which can take care of making the URL absolute for you.

private void LoadNavigation()
{
    Uri urlPrefix = new Uri("http://mysite.com");
    string xmlFilePath = ConfigurationManager.AppSettings["NavigationUrl"].ToString();
    XmlDocument doc = new XmlDocument();
    doc.Load(xmlFilePath);
    XmlNodeList navigationItems = doc.DocumentElement.FirstChild.ChildNodes;

    foreach (XmlNode item in navigationItems)    
    {
        Url fullUrl = new Uri(urlPrefix, item.Attributes.GetNamedItem("url").Value);
        String title = item.Attributes.GetNamedItem("title").Value;
        navList.InnerHtml += "<li> <a href=\"" + fullUrl + "\">" + title + "</a>";

        if (item.NextSibling == null)
        {
            navList.InnerHtml += "</a></li>"; //last item in list
        }
        else
        {
            navList.InnerHtml += "</a>|</li>"; //not the last item in list
        }
    }    
}

Answer 2

I would look into using XSLT to transform the XML into the required HTML.

So the code above becomes much simpler and would just have to load the XML file, load the XSL file, apply the transform and the result would be the HTML you require.
Another advantage is that if one day you decide to change the resulting HTML, this code will not need to change, only the XSL file will change.

A quick search yields a lot of useful resources including an article that seems to explain what I was suggesting: Transforming XML with XSLT and ASP

Another useful resource I'd recommend in case you are not familiar with XSLT is the XSLT Tutorial at W3Schools.

Answer 3

I'd build upon @sepp2k's modification by separating the concerns of 1) getting the link data, 2) building the links, 3) combining links into a bulleted list, and 4) adding the bullets to navList as follows:

private void LoadNavigation()
{
    Uri urlPrefix = new Uri("http://mysite.com");
    string xmlFilePath = ConfigurationManager.AppSettings["NavigationUrl"].ToString();
    XmlDocument doc = new XmlDocument();
    doc.Load(xmlFilePath);
    XmlNodeList navigationItems = doc.DocumentElement.FirstChild.ChildNodes;

    var links = navigationItems
        .Cast<XmlNode>()
        .Select(x => new
            {
                FullUrl = new Uri(urlPrefix, x.Attributes.GetNamedItem("url").Value) , 
                Title = x.Attributes.GetNamedItem("title").Value
            })
        .Select(x => String.Format("<a href=\"{0}\">{1}</a>", x.FullUrl, x.Title))
        .ToArray();

    var bulletedLinks = "<li>" + String.Join("|</li><li>", links) + "</li>";
    navList.InnerHtml += bulletedLinks;
}

Next I'd separate the concerns of 1) getting the xmlFilePath from the ConfigurationManager, and 2) loading the navigations items, from the process of updating navList. I'd also pass in the urlPrefix string or fetch it from an application configuration setting instead of hard coding it.

private string GetNavigationUrlXmlFilePath()
{
    string xmlFilePath = ConfigurationManager.AppSettings["NavigationUrl"].ToString();
    return xmlFilePath;
}

private IEnumerable<XmlNode> LoadNavigationItems()
{
    var xmlFilePath = GetNavigationUrlXmlFilePath();
    XmlDocument doc = new XmlDocument();
    doc.Load(xmlFilePath);
    XmlNodeList navigationItems = doc.DocumentElement.FirstChild.ChildNodes;
    return navigationItems.Cast<XmlNode>();
}

private void LoadNavigation(Uri urlPrefix)
{
    var links = LoadNavigationItems()
        .Select(x => new
            {
                FullUrl = new Uri(urlPrefix, x.Attributes.GetNamedItem("url").Value) , 
                Title = x.Attributes.GetNamedItem("title").Value
            })
        .Select(x => String.Format("<a href=\"{0}\">{1}</a>", x.FullUrl, x.Title))
        .ToArray();

    var bulletedLinks = "<li>" + String.Join("|</li><li>", links) + "</li>";
    navList.InnerHtml += bulletedLinks;
}

I'd also pull "NavigationUrl" out to a single location so I wouldn't have to change a bunch of code when I want to change the key name.

public static class ApplicationConfigurationSettingsKeys
{
    public static class MySite
    {
        public static readonly string NavigationUrl = "NavigationUrl";
    }
}

Then I'd put GetNavigationUrlXmlFilePath and LoadNavigationItems behind interfaces so that they can be tested independently and LoadNavigation can be tested without having to go to the disk.

public interface IApplicationConfigurationSettingsProvider
{
    string GetSetting(string key);
}

public class ApplicationConfigurationSettingsProvider : IApplicationConfigurationSettingsProvider
{
    public string GetSetting(string key)
    {
        return ConfigurationManager.AppSettings[key].ToString();
    }
}

public interface INavigationLoader
{
    IEnumerable<XmlNode> LoadNavigationItems();
}

public class NavigationLoader : INavigationLoader
{
    private readonly IApplicationConfigurationSettingsProvider _applicationConfigurationSettingsProvider;

    public NavigationLoader(IApplicationConfigurationSettingsProvider applicationConfigurationSettingsProvider)
    {
        _applicationConfigurationSettingsProvider = applicationConfigurationSettingsProvider;
    }

    public IEnumerable<XmlNode> LoadNavigationItems()
    {
        var xmlFilePath = _applicationConfigurationSettingsProvider
            .GetSetting(ApplicationConfigurationSettingsKeys.MySite.NavigationUrl);
        var doc = new XmlDocument();
        doc.Load(xmlFilePath);
        var navigationItems = doc.DocumentElement.FirstChild.ChildNodes;
        return navigationItems.Cast<XmlNode>();
    }
}

leaving:

private readonly INavigationLoader _navigationLoader;
private void LoadNavigation(Uri urlPrefix)
{
    var links = _navigationLoader.LoadNavigationItems()
        .Select(x => new
            {
                FullUrl = new Uri(urlPrefix, x.Attributes.GetNamedItem("url").Value) , 
                Title = x.Attributes.GetNamedItem("title").Value
            })
        .Select(x => String.Format("<a href=\"{0}\">{1}</a>", x.FullUrl, x.Title))
        .ToArray();

    var bulletedLinks = "<li>" + String.Join("|</li><li>", links) + "</li>";
    navList.InnerHtml += bulletedLinks;
}

Next I'd pull the conversion of XmlNode data to an HTML link out where it can be tested independently, possibly to an extension method:

public static class XmlNodeExtensions
{
    public static string ToHtmlLink(this XmlNode node, Uri urlPrefix)
    {
        var fullUrl = new Uri(urlPrefix, node.Attributes.GetNamedItem("url").Value);
        var title = node.Attributes.GetNamedItem("title").Value;
        return String.Format("<a href=\"{0}\">{1}</a>", fullUrl, title);
    }
}

leaving:

private void LoadNavigation(Uri urlPrefix)
{
    var links = _navigationLoader.LoadNavigationItems()
        .Select(x => x.ToHtmlLink(urlPrefix))
        .ToArray();

    var bulletedLinks = "<li>" + String.Join("|</li><li>", links) + "</li>";
    navList.InnerHtml += bulletedLinks;
}

Finally, I'd pull out another method, possibly an extension, for building the bulleted list

public static class StringExtensions
{
    public static string ToHtmlBullets(this IEnumerable<string> items, string suffix)
    {
        var bulleted = "<li>"+String.Join((suffix??"")+"</li><li>",items.ToArray())+"</li>";
        return bulleted;
    }
}

leaving only:

private void LoadNavigation(Uri urlPrefix)
{
    var links = _navigationLoader.LoadNavigationItems()
        .Select(x => x.ToHtmlLink(urlPrefix))
        .ToHtmlBullets("|");

    navList.InnerHtml += links;
}

This method might now be more accurately named something like AddNavigationLinks. I'm also inclined to make this method side-effect free by passing navList into the method, I just don't happen to know what its type is.