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?

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? — Ben Aaronson
– Ben Aaronson, Commented Oct 1, 2014 at 16:41
@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. — DGibbs
– DGibbs, Commented Oct 2, 2014 at 9:08
@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 — Ben Aaronson
– Ben Aaronson, Commented Oct 2, 2014 at 9:27
@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. — DGibbs
– DGibbs, Commented Oct 2, 2014 at 9:34

Ben Aaronson · Accepted Answer · 2014-10-01 16:41:16Z

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.

Nick Udell · Accepted Answer · 2014-10-01 15:33:24Z

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.

Stack Exchange Network

Determining if a discount is valid for a product

2 Answers 2

You must log in to answer this question.

Hot Network Questions

Determining if a discount is valid for a product

2 Answers 2

You must log in to answer this question.

Related

Hot Network Questions