5
\$\begingroup\$

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.

\$\endgroup\$
7
  • \$\begingroup\$ Welcome to codereview. In the future please use the code button (the one with the 1s and 0s) to format your code or indent it by four spaces. The <pre> tag does not enable syntax highlighting (though <code> does, but don't use that either) and more importantly: It does not escape HTML special characters. So your code did not display correctly using <pre> tags. Thanks. \$\endgroup\$
    – sepp2k
    Mar 1, 2011 at 17:11
  • \$\begingroup\$ Thanks for fixing it! The code button doesn't work very well for me. When I click the code button and then paste in my code, it only picks up the first line, then makes a mess of the rest of it. What am I missing? \$\endgroup\$
    – Josh Earl
    Mar 1, 2011 at 17:26
  • \$\begingroup\$ @Josh: First paste your code, then select it, then hit the code button. \$\endgroup\$
    – sepp2k
    Mar 1, 2011 at 17:40
  • \$\begingroup\$ Great. I'll try that next time. \$\endgroup\$
    – Josh Earl
    Mar 1, 2011 at 18:12
  • \$\begingroup\$ What is navList? A class variable? \$\endgroup\$
    – Michael K
    Mar 1, 2011 at 18:44

3 Answers 3

3
\$\begingroup\$

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
        }
    }    
}
\$\endgroup\$
4
  • \$\begingroup\$ Good catch on the issue about only checking if the node is the last if the URL is relative. This code was originally written by looking at the existing XML file. Currently, the last node is relative, so why would we need to check the absolute nodes? :) That's why I'm reworking this... \$\endgroup\$
    – Josh Earl
    Mar 1, 2011 at 20:55
  • \$\begingroup\$ I like the suggestion to use the Uri class here, but wouldn't it lead to the hardcoded absolute URLs getting the protocol and domain appended to them twice: mysite.comhttp://mysite.com/restofurl \$\endgroup\$
    – Josh Earl
    Mar 1, 2011 at 21:00
  • \$\begingroup\$ @Josh: No, new Uri(baseUri, uri) only prepends baseUri to uri if uri is relative. If it is absolute, baseUri is ignored. So it's perfect if you have an URL which might be relative or absolute and you want to turn it into an absolute one. \$\endgroup\$
    – sepp2k
    Mar 1, 2011 at 21:02
  • \$\begingroup\$ That is pretty sweet. Definitely implementing this piece of your suggestion at least. \$\endgroup\$
    – Josh Earl
    Mar 1, 2011 at 21:23
3
\$\begingroup\$

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.

\$\endgroup\$
1
  • \$\begingroup\$ This is an interesting suggestion. I've been wanting to dig into XSLT a bit anyway, so I might give it a shot. Would I be able to handle the absolute/relative URL issue in the XSL file? \$\endgroup\$
    – Josh Earl
    Mar 1, 2011 at 19:13
2
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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