I was asked to review NAppUpdate, a simple framework for providing auto-update support to .NET applications, available here. Just for to make things more interesting, the project leader of NAppUpdate is Itamar, who works for Hibernating Rhinos, and we actually use NAppUpdate in the profiler.

I treated it as an implementation detail and never actually looked at it closely before, so this is the first time that I am actually going over the code. On first impression, there is nothing that makes me want to hurl myself to the ocean from a tall cliff:

Let us dig deeper, and almost on the first try, we hit something that I seriously dislike.

Which leads to this:

public static class Errors
{
    public const string UserAborted = "User aborted";
    public const string NoUpdatesFound = "No updates found";
}

And I wouldn’t mind that, except that those are used like this:

There are actually quite a lot of issues with this small code sample. To start with, LastestError? Seriously?!

LatestError evoke strong memories of GetLastError() and all the associated fun with that.

It doesn’t give you the ability to multiple errors, and it is a string, so you can’t put an exception into it (more on that later).

Also, note how this work with the callback and the return code. Both of which have a boolean for success/failure. That is wrong.

That sort of style was valid for C, but .NET, we actually have exceptions, and they are actually quite a nice way to handle things.

Worse than that, it means that you have to check the return value, then go the LatestError and check what is going on there, except… what happen if there was an actual error?

Note the todo, it is absolutely correct. You really can’t just call a ToString() on an exception and get away with it (although I think that you should). There are a lot of exceptions where you would simply won’t get the required information. ReflectionException, for example, ask you to look at the LoaderExceptions property, and there are other such things.

In the same error handling category, this bugs me:

This is an exception that implements the serialization exception, but doesn’t have the [Serializable] attribute, which all exceptions should have.

Moving on, a large part of what NAU does is to check remote source for updates, then apply it. I liked this piece of code:

public class AppcastReader : IUpdateFeedReader
{
    // http://learn.adobe.com/wiki/display/ADCdocs/Appcasting+RSS

    #region IUpdateFeedReader Members

    public IList<IUpdateTask> Read(string feed)
    {
        XmlDocument doc = new XmlDocument();
        doc.LoadXml(feed);
        XmlNodeList nl = doc.SelectNodes("/rss/channel/item");

        List<IUpdateTask> ret = new List<IUpdateTask>();

        foreach (XmlNode n in nl)
        {
            FileUpdateTask task = new FileUpdateTask();
            task.Description = n["description"].InnerText;
            //task.UpdateTo = n["enclosure"].Attributes["url"].Value;
            task.UpdateTo = n["enclosure"].Attributes["url"].Value;

            FileVersionCondition cnd = new FileVersionCondition();
            cnd.Version = n["appcast:version"].InnerText;
            task.UpdateConditions.AddCondition(cnd, BooleanCondition.ConditionType.AND);

            ret.Add(task);
        }

        return ret;
    }

    #endregion
}

This is integrating with an external resources, and I like that this is simple to read and understand. I don’t have a lot of infrastructure going on in here that I have to deal with just to get what I want done.

There is a more complex feed reader for an internal format that allows you to use the full option set of NAU, but it is a bit complex (it does a lot more, to be fair), and again, I dislike the error handling there.

Another thing that bugged me on some level was this code:

The issue, and I admit that this is probably academic, is what happen if the string is large. I try to avoid exposing API that might force users to materialize a large data set in memory. This has implications on the working set, the large object heap, etc.

Instead, I would probably exposed a TextReader or even a Stream.

Coming back to the error handling, we have this:

FileDownloader fd = null;
if (Uri.IsWellFormedUriString(url, UriKind.Absolute))
    fd = new FileDownloader(url);
else if (Uri.IsWellFormedUriString(baseUrl, UriKind.Absolute))
    fd = new FileDownloader(new Uri(new Uri(baseUrl, UriKind.Absolute), url));
else if (Uri.IsWellFormedUriString(new Uri(new Uri(baseUrl), url).AbsoluteUri, UriKind.Absolute))
    fd = new FileDownloader(new Uri(new Uri(baseUrl), url));

if (fd == null)
    throw new ArgumentException("The requested URI does not look valid: " + url, "url");

My gripe is with the last line. Instead of doing it like this, I would have created a new Uri and let it throw the error. It is likely that it will have much more accurate error information about the actual reason this Uri isn’t valid.

On the gripping hand, we have this guy:

This is the abstraction that NAU manages, the Prepare() method is called to do all the actual work (downloading files, for example) and Execute() is called when we are done and just want to do the actual update.

I know that I am harping about this, but this is really important. Take a look at another error handling issue (this is representative of the style of coding inside this particular class) in the RegistryTask:

So, I have an update that is failing at a customer site. What error do I have? How do I know what went wrong?

Worse, here is another snippet from the FileUpdateTask:

For some things, an exception is thrown, for other, we just return false.

I can’t follow the logic for this, and trying to diagnose issues in production can be… challenging.

In summary, I went through most of the actual important bits of NAppUpdate, and like in most reviews, I focused on the stuff that can be improved. I didn’t touch on the stuff it does well, and that is its job.

We have been using NAppUpdate for the last couple of years, with very few issues. It quite nicely resolves the entire issue of auto updates to the point were in our code, we only need to call a few methods and it takes cares of the entire process for us.

Major recommendations from this codebase:

• Standardize the error handling approach. It is important.
• And even more important, logging is crucial to be able to diagnose issues in the field. NAU should log pretty much everything it does and why it does it.

This will allow later diagnosal of issues with relative ease, vs. “I need you to reproduce this and then break into the debugger”.