4
\$\begingroup\$

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?

\$\endgroup\$
  • \$\begingroup\$ I don't fully understand the refactored method. What does IncludesCategoriesAndBrands mean, and why is that being returned? Why is true returned if no rules match? \$\endgroup\$ – Ben Aaronson Oct 1 '14 at 16:41
  • \$\begingroup\$ @BenAaronson IncludesCategoriesAndBrands is a property of DiscountRule. It's an && of CategoryInclude and BrandInclude, the include/exclude flags. If a rule matches a product, it can only be valid if both CategoryInclude and BrandInclude. You raise an excellent point about returning true when no rules match, looks like a bug to me. I suspect the logic should be "if no rules, return true". Good spot. It also highlights another issue in not checking rules if a match occurs on a single ID, e.g. if only the brand matches, it should check whether this is include/exclude or not. \$\endgroup\$ – DGibbs Oct 2 '14 at 9:08
  • \$\begingroup\$ @DGibbs- Now that I understand that, maybe there's another bug: if a product matches a DiscountRule's CategoryId, but not its BrandId, but the DiscountRule has CategoryInclude true and BrandId false, shouldn't that give the result true rather than false? Also why would no rules return true? A rule seems like something inclusionary, rather than exclusionary. As in, adding a new rule could only add (brandId, categoryId) pairs which would be discounted (or leave them the same), never remove any \$\endgroup\$ – Ben Aaronson Oct 2 '14 at 9:27
  • \$\begingroup\$ @BenAaronson Yes, that's the other issue I mentioned at the end of the comment. No rules would return true as a DiscountRule is not required on a discount, if it has no restrictions it should just be true by default. \$\endgroup\$ – DGibbs Oct 2 '14 at 9:34
3
\$\begingroup\$

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.

\$\endgroup\$
1
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.