Determining if a discount is valid for a product

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 pa

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.

StackExchange Code Review Q#64406, answer score: 3