The wages of sinProper and improper usage of abstracting an OR/M
time to read 6 min | 1193 words
Originally posted at 3/11/2011

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:

image

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.

Tweet Share Share 30 comments
Tags:

More posts in "The wages of sin" series:

  1. (24 Mar 2011) Hit that database one more time…
  2. (23 Mar 2011) Inverse leaky abstractions
  3. (22 Mar 2011) Proper and improper usage of abstracting an OR/M
  4. (21 Mar 2011) Re-creating the Stored Procedure API in C#
  5. (18 Mar 2011) Over architecture in the real world