To read a book thou should have it bought, to buy a book thou should have it found! (Search Abstraction)

Question

I started a hobby project to find the books that I read (paperbacks) so that I can keep track of them (reading start date, finish date, number of pages, etc..)

The first challenge was to find the book on the net, and I realized there were different sources which you can search from, like google books (provides a JSON API), Amazon, isbnsearch.org (isbn search) etc.

I also want to cache the search results in my database so that I can search the books in my database first (to speed things up and not to exhaust API limits).

I then implemented something, and then abstracted something, then implemented and abstracted and ...

Finally, I came up with a generic search abstraction instead of something that searched for books only.

Any suggestions are welcome. I might be missing some pattern that covers all of this. I know this pattern is similar to the abstract factory, but maybe there are ways or patterns that I can use to simplify things more.

Here is the abstraction I finally came up with:

That is, for every different search source I need to implement 3 classes whose responsibilities are very well defined and very very simple. This is the most convenient way that I could achieve supporting modularity and testability.

Here is the source code for the interfaces:

public interface ISearchClient<SourceResponseType>
{
    SourceResponseType ExecuteRemoteSearch(string query);
}

public interface ISearchResponseParser<SourceResponseType, SourceItemType>
{
    IEnumerable<SourceItemType> ParseResponse(SourceResponseType searchResponse);
}

public interface ISearchItemConverter<SourceItemType, FinalItemType>
{
    FinalItemType Convert(SourceItemType sourceItem);
}

And here is one functional abstract class that brings these 3 together:

public abstract class SearchProvider<SourceResponseType, SourceItemType, FinalItemType>
{
    protected abstract ISearchClient<SourceResponseType> CreateClient();
    protected abstract ISearchResponseParser<SourceResponseType, SourceItemType> CreateParser();
    protected abstract ISearchItemConverter<SourceItemType, FinalItemType> CreateConverter();

    public virtual IEnumerable<FinalItemType> Search(string query)
    {
        ISearchClient<SourceResponseType> searchClient = CreateClient();
        SourceResponseType searchResponse = searchClient.ExecuteRemoteSearch(query);

        ISearchResponseParser<SourceResponseType, SourceItemType> searchResponseParser = CreateParser();
        IEnumerable<SourceItemType> searchSourceItems = searchResponseParser.ParseResponse(searchResponse);

        ISearchItemConverter<SourceItemType, FinalItemType> searchItemConverter = CreateConverter();

        List<FinalItemType> searchResults = new List<FinalItemType>();

        foreach (SourceItemType sourceItem in searchSourceItems)
        {
            FinalItemType finalItem = searchItemConverter.Convert(sourceItem);
            searchResults.Add(finalItem);
        }

        return searchResults;
    }
}

Note: Paging is out of the scope right now

I also implemented some helper classes (Which I don't like to call helpers actually. I never liked the notion of helper classes with static methods, but these functional abstract classes are re-usable and can be used by the implementations so I call them helpers)

For Http Get Clients:

public abstract class HttpGetSearchClient : ISearchClient<string>
{
    protected abstract string SearchUriTemplate { get; }

    protected virtual WebClient GetHttpClient()
    {
        return new WebClient();
    }

    public virtual string ExecuteRemoteSearch(string query)
    {
        string urlEncodedQuery = HttpUtility.UrlEncode(query);
        string formattedQueryUrl = string.Format(SearchUriTemplate, urlEncodedQuery);
        return GetHttpClient().DownloadString(formattedQueryUrl);
    }
}

For Parsing Http Get response using HtmlAgilityPack:

public abstract class HtmlResponseParser : ISearchResponseParser<string, HtmlNode>
{
    protected abstract string SourceItemXPathQuery { get; }

    public IEnumerable<HtmlNode> ParseResponse(string searchResponse)
    {
        HtmlAgilityPack.HtmlDocument htmlDocument = new HtmlAgilityPack.HtmlDocument();
        htmlDocument.LoadHtml(searchResponse);
        HtmlNodeCollection bookNodes = htmlDocument.DocumentNode.SelectNodes(SourceItemXPathQuery);
        return bookNodes;
    }
}

Then, I came up with this to cover an actual search provider so that the only thing a search provider (a provider which searched a specific source) needs is to implement 3 concrete classes, 1 SearchClient, 1 ResponseParser and 1 ItemConverter:

public abstract class SearchProvider<SourceResponseType, SourceItemType, FinalItemType, ClientType, ParserType, ConverterType>
    : SearchProvider<SourceResponseType, SourceItemType, FinalItemType>
    where ClientType : ISearchClient<SourceResponseType>, new()
    where ParserType : ISearchResponseParser<SourceResponseType, SourceItemType>, new()
    where ConverterType : ISearchItemConverter<SourceItemType, FinalItemType>, new()
{
    protected override ISearchClient<SourceResponseType> CreateClient()
    {
        return new ClientType();
    }

    protected override ISearchResponseParser<SourceResponseType, SourceItemType> CreateParser()
    {
        return new ParserType();
    }

    protected override ISearchItemConverter<SourceItemType, FinalItemType> CreateConverter()
    {
        return new ConverterType();
    }
}

And rest, is just FUN!

Here is a sample implementation for isbnsearch.org:

public class Book : Lab.Book
{
}

public class SearchClient : HttpGetSearchClient
{
    protected override string SearchUriTemplate
    {
        get { return "http://www.isbnsearch.org/isbn/{0}"; }
    }
}

public class ResponseParser : HtmlResponseParser
{

    protected override string SourceItemXPathQuery
    {
        get { return "//div[@id='book']"; }
    }
}

public class ItemConverter : ISearchItemConverter<HtmlNode, IBook>
{
    public IBook Convert(HtmlNode sourceItem)
    {
        Book newBook = new Book();
        HtmlNode thumbnailNode = sourceItem.SelectSingleNode("div[@class='thumbnail']");
        if (thumbnailNode != null)
        {
            newBook.ThumbnailUri = thumbnailNode.SelectSingleNode("img").Attributes["src"].Value;
        }
        HtmlNode titleNode = sourceItem.SelectSingleNode("div[@class='bookinfo']/h2");
        if (titleNode != null)
        {
            newBook.Title = titleNode.InnerText;
        }
        foreach (HtmlNode pStrongNode in sourceItem.SelectNodes("div[@class='bookinfo']/p/strong"))
        {
            if (pStrongNode.InnerText == "ISBN-13:")
            {
                newBook.ISBN13 = pStrongNode.ParentNode.SelectSingleNode("a").InnerText;
            }
            else if (pStrongNode.InnerText == "ISBN-10:")
            {
                newBook.ISBN10 = pStrongNode.ParentNode.SelectSingleNode("a").InnerText;
            }
            else if (pStrongNode.InnerText == "Author:")
            {
                newBook.Author = pStrongNode.ParentNode.InnerText.Replace("Author: ", "");
            }
            else if (pStrongNode.InnerText == "Publisher:")
            {
                newBook.Publisher = pStrongNode.ParentNode.InnerText.Replace("Publisher: ", "");
            }
            else if (pStrongNode.InnerText == "Published:")
            {
                newBook.PublishDate = pStrongNode.ParentNode.InnerText.Replace("Published: ", "");
            }
        }
        return newBook;
    }
}

And finally, isbnsearch.org search provider is completed by defining: (SearchClient, ResponseParser, ItemConverter are the classes implemented above, think of these as in a plug-in assembly, they have their own namespace)

    public class SearchProvider : SearchProvider<string, HtmlNode, IBook, SearchClient, ResponseParser, ItemConverter>
    {

    }

And here is the IBook interface:

public interface IBook
{
    Guid Id { get; set; }
    string Title { get; set; }
    string Description { get; set; }
    string ThumbnailUri { get; set; }
    string ISBN10 { get; set; }
    string ISBN13 { get; set; }
    string Author { get; set; }
    string Publisher { get; set; }
    string PublishDate { get; set; }
}

public class Book : IBook
{
    public virtual Guid Id { get; set; }
    public virtual string Title { get; set; }
    public virtual string Description { get; set; }
    public virtual string ThumbnailUri { get; set; }
    public virtual string ISBN10 { get; set; }
    public virtual string ISBN13 { get; set; }
    public virtual string Author { get; set; }
    public virtual string Publisher { get; set; }
    public virtual string PublishDate { get; set; }
}

Answer 1

You said you passed through few iterations of distilling your framework, but did not mention what guided your refactorings.

I would expect most of the effort of such distillation to contribute to the model you use. As I understand, it's now only capable to query by a string, without a capability to query by more specific terms, such as ISBN, Title, Author and date range.

I would suggest to look into Specification Pattern.

Eric Evans talks about this pattern and it's application in the context of a query service in his Domain-Driven Design: Tackling Complexity in the Heart of Software

Another important target of distillation is removing unnecessary dependencies in the model and the plugin interface. You left the plugin interface out of the scope of this research, but I'd say it's important too.

Dependencies on specialized frameworks such as the Agility Pack seem severe in this light. It's an implementation detail, and i see no reason to bring the types defined in external dependencies to the base classes of the framework:

public IEnumerable<HtmlNode> ParseResponse(string searchResponse)

In your design you rely on abstract implementations heavily, so your base framework contains useful implementations. It may seem useful, but it actually entangles the design or rather makes it ambiguous.

For example, if some other gateway implementation would need to work over json instead of html, a base class which is coupled to the Agility Pack makes no sense anymore. In this case there are two options: implement another base class similar to your HtmlResponseParser and favor your design, or ignore your design, and implement the other way. In other words, favor Composition over inheritance

To favor composition over inheritance is a design principle that gives the design higher flexibility, giving business-domain classes and more stable business domain in the long term. In other words, HAS-A can be better than an IS-A relationship.

You can still benefit from nice reusable productivity types in form of helper classes, not base classes:

Contracts -> Depends on nothing
Particular Implementation 1 -> Depends on Contracts, Agility Pack
Particular Implementation 2 -> Depends on Contracts, Json.Net

Another issue with this kind of coupling is that in your plugin architecture you make all this irrelevant implementation details visible to the plugin framework.

It makes sense to hide the entire thing behind a non-generic interface:

interface IBookSearchGateway
{
    IEnumerable<IBook> Query(IBookSpecification specification);
}

As for the factoring of some of the abstractions, it makes sense to make abstractions more powerful without the implementation details leaking into the abstraction:

protected override string SearchUriTemplate
{
    get { return "http://www.isbnsearch.org/isbn/{0}"; }
}

A string format in this case is an implementation detail. You couple your design to the language of string.Format method. This language is not bad in itself, and it would be useful in other contexts, such as User Interface: "Your search returned {0} results from {1} sources"

It would be a more powerful abstraction:

protected override string GetSearchUrl(IBookSpecification spec)
{
    return string.Format("http://www.isbnsearch.org/isbn/{0}", spec.Isbn);
}

As for the new constraint used in generic base implementation, i think it could be too restrictive. I would suggest removing this constraint, and make the factory methods abstract, with the corresponding interface types instead of concrete classes.

This will allow to drop the three last type parameters from the SearchProvider<`6> (ClientType, ParserType, ConverterType)

SearchProvider<SourceResponseType, SourceItemType, FinalItemType, ClientType, ParserType, ConverterType>

turns to:

BaseSearchProvider<SourceResponseType, SourceItemType, FinalItemType>

Search method could be reduced to just one line:

return CreateParser().ParseResponse(CreateClient().ExecuteRemoteSearch(query)).Select(CreateConverter().Convert).ToList();

...which makes me thinking that the algorithm is so simple that it's not worth to create a base class for this sole purpose:

public abstract class BaseSearchProvider<SourceResponseType, SourceItemType, FinalItemType>
{
    protected abstract ISearchClient<SourceResponseType> CreateClient();
    protected abstract ISearchResponseParser<SourceResponseType, SourceItemType> CreateParser();
    protected abstract ISearchItemConverter<SourceItemType, FinalItemType> CreateConverter();

    public virtual IEnumerable<FinalItemType> Search(string query)
    {
        return CreateParser().ParseResponse(CreateClient().ExecuteRemoteSearch(query)).Select(CreateConverter().Convert).ToList();
    }
}

Again, in favor of composition over inheritance, it's better to encapsulate this algorithm as a Strategy pattern implementation.

As for general code quality, I would suggest using switch construct instead of cascading else if:

switch(pStrongNode.InnerText)
{
    ...
}

I would also note that evaluating pStrongNode.InnerText multiple times could be sub-optimal, but it's already fixed with a switch usage.

I don't know the Agality Pack framework very well, but there are probably a more performant way to parse a node then navigating through the ParentNode property with subsequent select: pStrongNode.ParentNode.SelectSingleNode.

Therefore:

• Enrich your model with the Specification Pattern distilling your model from implementation details.
• Hide implementation details behind non-generic gateway interface.
• Use Strategy instead of base class in favor of Composition over inheritance.
• Watch for leaky abstractions (that string.Format thing).
• Use the write language constructs (switch vs else if).
• Watch for potential performance issues in multiple accesses to one property.
• Watch the underlying framework's opportunities.