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.

You might have wondered why I named this blog series the way I did, I named it because of the method outline below. Please note that I had to invent a new system to visualize the data access behavior on this system:

• In red, we have queries that are executed once: 3 queries total.
• In aqua, we have queries that are executed once for each item in the order: 2 queries per each product in the order.
• In purple, we have queries that are executed once for each attribute in each of the products in the order: 1 query per attribute per product in the order.

Now, just to give you some idea, let us say that I order 5 items, and each item have 5 attributes…

We get the following queries:

• 3 queries – basic method cost
• 10 queries – 2 queries per each product
• 25 queries – 1 query for each attribute for each product

Totaling in 38 queries for creating a fairly simple order. After seeing this during the course review of the application, I have used that term for this method, because it is almost too easy to make those sort of mistakes.

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.

As you might have noticed, I am pretty big fan of the NHibernate Futures feature (might be because I wrote it ), so I was encouraged when I saw it used in the project. It can drastically help the performance of the system.

Except, that then I saw how it is being used.

Which is then called from CurrencyService:

Which is then called…

Do you see that? We take the nice future query that we have, and turn it into a standard query, blithely crashing any chance to actually optimize the data access of the system.

The problem is that from the service API, there is literally nothing to tell you that you need to order your calls so all the data access happens up front, and it is easy to make mistakes such as this. The problem is that there is meaning lost at any additional abstraction layer, and pretty soon whatever good intention you had when you wrote a particular layer, 7 layers removed, you can’t really remember what is the best way to approach something.

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:

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.

Originally posted at 3/10/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. The problem is that this system seems to be drastically more complicated than it should be.

In this case, I want to look at the type of API that is exposed:

If this reminds you of the bad old days of having only Stored Procedure API available, that is not by chance. Far worst than that, however, is the call paths where this is used.

• IEmailTemplateRepository.Get(EmailTemplateLookup) is implemented as
  

  public EmailTemplate Get(EmailTemplateLookup emailId)
  {
      return Get((int)emailId);
  }
  and is only used in:

  • EmailService.Get(EmailTemplateLookup) whose implementation is:
    

    public EmailTemplate GetEmail(EmailTemplateLookup template)
    {
        return emailTemplateRepository.Get(template);
    }
    

• ICategoryRepository.GetParentCategories is only used from:
  • CategoryService.GetParentCategories which is implemented as:
    

    public IEnumerable<Category> GetParentCategories()
    {
        IEnumerable<Category> categories = categoryRepository.GetParentCategories();
    
        return categories;
    }

For that matter, let us take a look at the entire CurrencyService class, shall we?

public class CategoryService : ICategoryService
{
    private readonly ICategoryRepository categoryRepository;

    public CategoryService(ICategoryRepository categoryRepository)
    {
        this.categoryRepository = categoryRepository;
    }

    public IList<Category> GetCategories()
    {
        return categoryRepository.GetAll();
    }

    public Category GetCategory(int id)
    {
        return categoryRepository.Get(id);
    }

    public void SaveOrUpdate(Category categoryModel)
    {
        categoryRepository.SaveOrUpdate(categoryModel);
    }

    public void Delete(Category category)
    {
        categoryRepository.Delete(category);
    }

    public IEnumerable<Category> GetParentCategories()
    {
        IEnumerable<Category> categories = categoryRepository.GetParentCategories();

        return categories;
    }
}

To be honest, I really don’t see the point.

Now, just a hint on the next few posts, there are places where I think wrapping the usage of the NHibernate API was a good idea, even if I strongly disagree with how this was done.

Originally posted at 3/10/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. The problem is that this system seems to be drastically more complicated than it should be.

I am going to focus  on different parts of the system in each of those posts. In this case, I want to focus on the very basis for the application data access:

Are you kidding me? This is before you sit down to write a single line of code, mind you. Just the abstract definitions for everything makes my head hurt.

It really hits you over the head when you get this trivial implementation:

public class EmailTemplateRepository : Repository<EmailTemplate>, IEmailTemplateRepository
{
    public EmailTemplate Get(EmailTemplateLookup emailId)
    {
        return Get((int)emailId);
    }
}

Yes, this is the entire class.  I am sorry, but I really don’t see the point. The mental weight of all of this is literally crashing.