Share via

Bookmark Collection: Storage Refactoring

  • Article

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]
{
get
{
if (!dictionary.ContainsKey(label))
return null;

        return dictionary[label].Uri;
}

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