The wages of sin: Proper and improper usage of abstracting an OR/M

This time, this is a review of the Sharp Commerce application. Again, I have stumbled upon the application by pure chance, and I have very little notion about who wrote it.

In this case, I want to focus on the ProductRepository:



In particular, those methods also participate in Useless Abstraction For The Sake Of Abstraction Anti Pattern. Here is how they are implemented:

public AttributeItem GetAttributeItem(int attributeItemId)
{
    return Session.Get<AttributeItem>(attributeItemId);
}

public Attribute GetAttribute(int attrbuteId)
{
    return Session.Get<Attribute>(attrbuteId);
}

public IEnumerable<Attribute> GetAllAttributes()
{
    return Session.QueryOver<Attribute>()
        .Future<Attribute>();
}

public void SaveOrUpdate(Attribute attribute)
{
    Session.SaveOrUpdate(attribute);
}

And here is how they are called (from ProductService):

public AttributeItem GetAttributeItem(int attributeItemId)
{
    return productRepository.GetAttributeItem(attributeItemId);
}

public Attribute GetAttribute(int attrbuteId)
{
    return productRepository.GetAttribute(attrbuteId);
}

public void SaveAttribute(Attribute attribute)
{
    productRepository.SaveOrUpdate(attribute);
}

 public IList<Product> GetProducts()
 {
     return productRepository.GetAll();
 }

 public Product GetProduct(int id)
 {
     return productRepository.Get(id);
 }

 public void SaveOrUpdate(Product product)
 {
     productRepository.SaveOrUpdate(product);
 }

 public void Delete(Product product)
 {
     productRepository.Delete(product);
 }

 public IEnumerable<Attribute> GetAllAttributes()
 {
     return productRepository.GetAllAttributes();
 }

Um… why exactly?

But as I mentioned, this post is also about the proper usage of abstracting the OR/M. A repository was originally conceived as a to abstract away messy data access code into nicer to use code. The product repository have one method that actually do something meaningful, the Search method:

public IEnumerable<Product> Search(IProductSearchCriteria searchParameters, out int count)
{
    string query = string.Empty;
    if (searchParameters.CategoryId.HasValue && searchParameters.CategoryId.Value > 0)
    {
        var categoryIds = (from c in Session.Query<Category>()
                           from a in c.Descendants
                           where c.Id == searchParameters.CategoryId
                           select a.Id).ToList();

        query = "Categories.Id :" + searchParameters.CategoryId;
        foreach (int categoryId in categoryIds)
        {
            query += " OR Categories.Id :" + categoryId;
        }
    }

    if (!string.IsNullOrEmpty(searchParameters.Keywords))
    {
        if (query.Length > 0)
            query += " AND ";

        query += string.Format("Name :{0} OR Description :{0}", searchParameters.Keywords);
    }

    if (query.Length > 0)
    {
        query += string.Format(" AND IsLive :{0} AND IsDeleted :{1}", true, false);

        var countQuery = global::NHibernate.Search.Search.CreateFullTextSession(Session)
            .CreateFullTextQuery<Product>(query);

        var fullTextQuery = global::NHibernate.Search.Search.CreateFullTextSession(Session)
            .CreateFullTextQuery<Product>(query)
            .SetFetchSize(searchParameters.MaxResults)
            .SetFirstResult(searchParameters.PageIndex * searchParameters.MaxResults);

        count = countQuery.ResultSize;

        return fullTextQuery.List<Product>();
    }
    else
    {
        var results = Session.CreateCriteria<Product>()
            .Add(Restrictions.Eq("IsLive", true))
            .Add(Restrictions.Eq("IsDeleted", false))
            .SetFetchSize(searchParameters.MaxResults)
            .SetFirstResult(searchParameters.PageIndex * searchParameters.MaxResults)
            .Future<Product>();

        count = Session.CreateCriteria<Product>()
            .Add(Restrictions.Eq("IsLive", true))
            .Add(Restrictions.Eq("IsDeleted", false))
            .SetProjection(Projections.Count(Projections.Id()))
            .FutureValue<int>().Value;

        return results;
    }
}

I would quibble about whatever this is the best way to actually implement this method, but there is little doubt that something like this is messy. I would want to put this in a very distant corner of my code base, but it does provides a useful abstraction. I wouldn’t put it in a repository, though. I would probably put it in a Search Service instead, but that isn’t that important.

What is important is to understand where there is actually a big distinction between code that merely wrap code for the sake of increasing the abstraction level and code that provide some useful abstraction over an operation.