Determining if a discount is valid for a product

Question

I'm in the process of re-writing a method I've come across in our code base. It's pretty lengthy and a lot of it seemed redundant. It's a Discount object class member. It operates on the discounts DiscountRules property.

DiscountRule can have behave as follows:

• Can apply to all brands and a single category
• Can apply to all categories and a single brand
• Can apply to a single category and a single brand

In addition to these few simple rules, there's also "include/exclude" functionality.

Example rules:

• All brands (include) in Televisions (exclude)
• Pioneer (include) in All categories (exclude)
• Pioneer (exclude) in All categories (include)
• Pioneer (include) in Televisions (include)

etc...

This would make the discount valid for any brands not in the 'Televisions' category (1st example). This is figured out by looking at the Brand + Category ID that a product in the basket falls under, if they match, it checks that the rule has both BrandInclude and CategoryInclude (include/exclude flags) set for the rule (either one of these being false would invalidate the whole).

Here is the original method:

public bool IsDiscountValidForProduct(int brandID, int categoryID)
{
    foreach (DiscountRule rule in this.DiscountRules)
    {
        // Rule is all brands, no need to check brand id
        if (rule.AllBrands && rule.CategoryId == categoryID)
        {
            // Check is passed, now we just need to check the inclusion/exclusion rules
            if (rule.IncludesCategoriesAndBrands)
            {
                return true;
            }
            else if (!rule.IncludesCategoriesAndBrands)
            {
                return false;
            }
            else
            {
                // It's a mix of exclude + include
                if (rule.BrandInclude && !rule.CategoryInclude)
                {
                    // no need for brand lookup, it's all brands, just make sure categoryID from rule not = categoryID param
                    return rule.CategoryId != categoryID;
                }
                else if (rule.CategoryInclude && !rule.BrandInclude)
                {
                    // Since this is all brands, and brands are excluded, no need to check category, return false
                    return false;
                }
            }
        }
        // Rule is all categories, no need to check category id
        else if (rule.AllCategories && rule.BrandId == brandID)
        {
            if (rule.IncludesCategoriesAndBrands)
            {
                return true;
            }
            else if (!rule.IncludesCategoriesAndBrands)
            {
                return false;
            }
            else
            {
                // It's a mix of exclude + include - includes brand but not category
                if (rule.BrandInclude && !rule.CategoryInclude)
                {
                    // Rule is category exclude, since we're checking under all categories, no need for lookup, return false
                    return false;
                }
                else if (rule.CategoryInclude && !rule.BrandInclude)
                {
                    // rule is all categories, no need to check category id, just look at the brand
                    return rule.BrandId != brandID;
                }
            }
        }
        else if (rule.BrandId == brandID && rule.CategoryId == categoryID)
        {
            if (rule.IncludesCategoriesAndBrands)
            {
                return true;
            }
            // If either category or brand exclude or both, rule is invalid
            else if (!rule.IncludesCategoriesAndBrands || (!rule.CategoryInclude || !rule.BrandInclude))
            {
                return false;
            }
        }
   }
  return true;
}

Here is the refactored version:

public bool IsDiscountValidForProduct(int brandID, int categoryID)
{
    foreach (DiscountRule rule in this.DiscountRules)
    {
        if (rule.AllBrands && rule.CategoryId == categoryID 
            || rule.AllCategories && rule.BrandId == brandID 
            || rule.BrandId == brandID && rule.CategoryId == categoryID)
        {
            return rule.IncludesCategoriesAndBrands;
        }
    }

    return true;
}

How can I further improve this (short of adding comments)? What would you do differently and why?

Answer 1

First, it seems like the brand and the category checks are more or less separate from each other. It's confusing reading that if statement to work out that you're churning through every valid combination. So separate them out into their own methods:

public bool CategoryMatchesRule(DiscountRule rule, int categoryId)
{
    return rule.AllCategories || rule.CategoryId == categoryId;
}

Then do similar for the brand. Then your loop can just check:

if(BrandMatchesRule(rule, brandId) && CategoryMatchesRule(rule, categoryId))

---

A side benefit of this is that you're no longer expressing the business logic that a discount can't count for both all brands and all categories. This method consumes discount rules, so it shouldn't have to be coupled to understanding the business logic behind how those rules are created or validated. That's likely to be expressed somewhere else, so this is a form of repetition.

Along similar lines, we can refactor this to follow the Tell, Don't Ask principle by moving these method into the DiscountRule class. That class should have a public bool MatchesProduct(int brandId, int categoryId) method, and the CategoryMatchesRule and BrandMatchesRule methods can be private methods on DiscountRule. It makes sense for the rule to responsible itself for saying whether or not a product matches it.

Answer 2

You could use LINQ:

public bool IsDiscountValidForProduct(int brandID, int categoryID)
{
    var rule = DiscountRules.FirstOrDefault(rule=>
        (rule.AllBrands && rule.CategoryId == categoryID) || 
        (rule.AllCategories && rule.BrandId == brandID) || 
        (rule.BrandId == brandID && rule.CategoryId == categoryID));

    return rule != null? rule.IncludesCategoriesAndBrands : true;
}

I also like to group boolean clauses in brackets for clarity.