# Elegant approach to building unordered list from XML

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";
XmlDocument doc = new XmlDocument();

{
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.

• 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. Mar 1, 2011 at 17:11
• 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? Mar 1, 2011 at 17:26
• @Josh: First paste your code, then select it, then hit the code button. Mar 1, 2011 at 17:40
• Great. I'll try that next time. Mar 1, 2011 at 18:12
• What is navList? A class variable? Mar 1, 2011 at 18:44

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");
XmlDocument doc = new XmlDocument();

{
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
}
}
}

• 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... Mar 1, 2011 at 20:55
• 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 Mar 1, 2011 at 21:00
• @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. Mar 1, 2011 at 21:02
• That is pretty sweet. Definitely implementing this piece of your suggestion at least. Mar 1, 2011 at 21:23

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.

• 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? Mar 1, 2011 at 19:13

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");
XmlDocument doc = new XmlDocument();

.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();

}


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()
{
return xmlFilePath;
}

{
XmlDocument doc = new XmlDocument();
}

{
.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();

}


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 interface IApplicationConfigurationSettingsProvider
{
string GetSetting(string key);
}

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

{
}

{

{
_applicationConfigurationSettingsProvider = applicationConfigurationSettingsProvider;
}

{
var xmlFilePath = _applicationConfigurationSettingsProvider
var doc = new XmlDocument();
}
}


leaving:

private readonly INavigationLoader _navigationLoader;
{
.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();

}


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)
{
.ToArray();

}


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)
{