May 02, 2009

Put functionality where it belongs

In one of my recent BlogEngine.Net Experiment refactoring sessions, I found myself working on the BlogEngine.Core Search.cs file. Very quickly I noticed that the search functionality works on a “catalog” object, which is a local Collection<Post> type variable. Various methods work on this collection, “building" it, adding to it, and so forth.

The BuildCatalog() method looks like this:

private static void BuildCatalog()

{

OnIndexBuilding();

 

lock (_SyncRoot)

{

_Catalog.Clear();

foreach (Post post in Post.Posts)

{

if (!post.IsPublished)

continue;

 

AddItem(post);

if (BlogSettings.Instance.EnableCommentSearch)

{

foreach (Comment comment in post.Comments)

{

if (comment.IsApproved)

AddItem(comment);

}

}

}

 

foreach (Page page in Page.Pages)

{

if (page.IsPublished)

AddItem(page);

}

}

 

OnIndexBuild();

}

Notice how, in the first ForEach, all of the posts are requested (Post.Posts), but we only want to work on the published ones. So, we open up and inspect each post to see if it’s published before continuing.

We then do the same thing for Comments, getting them all but only working with the approved ones. And we do it yet again with Pages, getting them all when we only want the published ones.

There’s obviously some extra work going on here, but how should we refactor it?

One of the problems with this code is that the Catalog needs to know too much about the internals of a Post, a Comment, and a Page to do its work. The Catalog shouldn’t need to know that a Post and a Page will have an IsPublished property, nor that a Comment will have an IsApproved property. (See the definition of the Inappropriate Intimacy code smell.) It should simply be able to ask for published posts, approved comments, and published pages from the appropriate provider.

But who can the Catalog ask? Neither the Post class nor the Page class can provide the answer, so the Catalog does the best it can. The solution, then, is to fix the Post and Page classes so that they can provide the Published and Approved content themselves.

Once done, the BuildCatalog() method will look more like this:

private static void BuildCatalog()

{

OnIndexBuilding();

bool commentsEnabled = BlogSettings.Instance.EnableCommentSearch;

 

lock (_SyncRoot)

{

_Catalog.Clear();

foreach (Post post in Post.PublishedPosts)

{

AddItem(post);

if (commentsEnabled)

{

foreach (Comment comment in post.ApprovedComments)

{

AddItem(comment);

}

}

}

 

foreach (Page page in Page.PublishedPages)

{

AddItem(page);

}

}

 

OnIndexBuild();

}

The BuildCatalog() method is simpler and more straightforward. It’s also decoupled from the internals of the Post, Comment, and Page classes, in the sense that it doesn’t need to know how to determine if a post or page is published, it just asks for them.

In the next couple of posts I’ll walk through a few more refactorings of the ‘catalog’ functionality.