Bookmark Collection: Storage Refactoring

As I mentioned in the last post I did not like that I had to create a new Bookmark when iterating over the Bookmarks. Instead of creating a new Bookmark every time let’s store them in the dictionary. I need to change the declaration of the dictionary to the following:

private Dictionary<string, Bookmark> dictionary = 
                 new Dictionary<string, Bookmark>();

When I do this of course the BookmarkCollection code no longer compiles, the Add method, the indexer, and the GetEnumerator method no longer compile-let’s address the Add method first. The Add method does not compile because the value of the dictionary used to be a Uri and now it is of type Bookmark. Here is the updated code:

public void Add(string label, Uri site)
    if (label == null || site == null)
        throw new ArgumentNullException();

    Add(new Bookmark(label, site));

private void Add(Bookmark bookmark)
    dictionary[bookmark.Label] = bookmark;

That compiles, on to the indexer. Just like the Add method the indexer was assuming that the value of the KeyValuePair is a Uri. Here is the fix:

public Uri this[string label]
        if (!dictionary.ContainsKey(label))
            return null;

        return dictionary[label].Uri; 

        Add(label, value);

This now compiles as well. The last method to fix is the GetEnumerator. We no longer have to create a new Bookmark each time we can just return the value of the KeyValuePair.

public IEnumerator<Bookmark> GetEnumerator()
    foreach (KeyValuePair<string, Bookmark> bookmark in dictionary)
        yield return bookmark.Value;

Now the code compiles and all the tests pass. The refactoring is complete and we did not have to change the interface of the class or any of the tests. I hope this demonstrates some of the power of having a suite of tests. A change like this could have easily introduced a problem that was not detectable. Having the tests, even though they are incomplete, gives us a safety net which allows us to move forward with more confidence.




Comments (3)

  1. Thomas Eyde says:

    I guess it should be yield return bookmark.Uri?

    But why do you want a typesafe enumerator in the first place? Didn’t it just cause you problems when you add generics to the equation?

    And why didn’t you just iterate the List<>.Values collection and do a yield return on that:

    foreach(Uri uri in dictionary.Values) yield return uri;