Stop trying so hard to use pattern-matching

In the article Why tuples in C# are not always a code smell by Dennis Frühauff (dateo. Coding Blog), the author writes the following code for calculating a discount.

The requirements are as follows:

• Premium customers get 20% off.
• Gold customers get 30% off.
• Regular customers, when they are students (< 25 years), get 10% off.
• Regular adult customers get no discount.
• All regular customers get 15% off during happy hour (3 to 8 p.m.).

The author’s original version

public decimal CalculateDiscount(Customer customer, DateTime time)
{
    if (customer.CustomerType == CustomerType.Gold)
    {
        return 0.3m;
    }
    else if (customer.CustomerType == CustomerType.Premium)
    {
        return 0.20m;
    }
    else
    {
        if (time.Hour is > 15 and < 20)
        {
            return 0.15m;
        }

        if (customer.Age < 25)
        {
            return 0.1m;
        }
        else
        {
            return 0m;
        }
    }

    return 0m;
}

He doesn’t like this code, and neither do I. But we have different reasons.

The author’s pattern-matching version

The author rewrites the code above with pattern-matching, to make it “pretty much look like the business rules stated above”.

His final version looks like this:

public decimal CalculateDiscount(Customer customer, DateTime time)
{
    return (IsStudent(customer), IsHappyHour(time), customer.CustomerType) switch
    {
        (_,         _, CustomerType.Gold)    => 0.3m,
        (_,         _, CustomerType.Premium) => 0.2m,
        (_,      true, CustomerType.Regular) => 0.15m,
        (true,  false, CustomerType.Regular) => 0.10m,
        (false, false, CustomerType.Regular) => 0.0m
    };
}

public bool IsStudent(Customer customer) => customer.Age < 25;
public bool IsHappyHour(DateTime datetime) => datetime.Hour is > 15 and < 20;

I strongly disagree that this looks like the original business requirements. In order to figure out who gets a 15% discount, you have to figure out what the first two boolean fields of the tuple indicate, so you look at the ad-hoc-instantiated tuple (which is created only in order to pattern-match on it), where you can see from the local-method names that they indicate whether the customer is a student and whether the sale was made during happy hour, respectively.

I have a few issues with this version;

• As noted above, it is not easily legible
• I am not sure about the allocation or efficiency of this code
• The extra formatting required (aligning the _ placeholders) makes it look difficult to maintain

Cleaning up the original without pattern-matching

I would tackle this differently, and with classic means. First of all, my main problem with the original version is that it’s made unnecessarily long and cluttered by including else statements after returns. Get rid of those and you’ll get rid of indenting and all of a sudden, the original code looks remarkably legible. It’s also 100% clear that there are no allocations and we don’t have to worry our pretty heads about the efficiency of code generated for either if and return statements or for simple comparisons.

public decimal CalculateDiscount(Customer customer, DateTime time)
{
    if (customer.CustomerType == CustomerType.Gold)
    {
        return 0.3m;
    }
  
    if (customer.CustomerType == CustomerType.Premium)
    {
        return 0.20m;
    }

    if (time.Hour is > 15 and < 20)
    {
       return 0.15m;
    }

    if (customer.Age < 25)
    {
       return 0.1m;
    }

    return 0m;
}

Improving semantics

How much clearer would you like that to be? I suppose we could add some local methods to add some semantics to the comparisons.

public decimal CalculateDiscount(Customer customer, DateTime time)
{
    if (IsLevel(CustomerType.Gold))
    {
        return 0.3m;
    }
  
    if (IsLevel(CustomerType.Premium))
    {
        return 0.20m;
    }

    if (IsHappyHour())
    {
       return 0.15m;
    }

    if (IsStudent())
    {
       return 0.1m;
    }

    return 0m;

    bool IsLevel(CustomerType customerType) => customer.CustomerType == customerType;
    bool IsStudent() => customer.Age < 25;
    bool IsHappyHour() => time.Hour is > 15 and < 20;
}

To make up for the fact that we lost all of that delicious pattern-matching and those tuples from the author’s version, we’re using local methods. Is this an improvement? Overall, I think so. The first version was already pretty good, but now we’ve improved the semantics by taking the guesswork out of the magic numbers. The IsHappyHour method is definitely an improvement. The IsStudent also imparts more knowledge about what the magic age of 25 means. Also, we’ve managed to separate the calculation of the rebate from the determination of the conditions that affect the rebate.

Pattern-matching: take two?

Can we do anything with pattern-matching, though? Can we use pattern-matching in a way that’s more legible than the version proposed by the author?

What about this?

public static decimal CalculateDiscount(this Customer customer, DateTime time)
{
    return (customer, time) switch
    {
        ({ CustomerType: CustomerType.Gold }, _) => 0.3m,
        ({ CustomerType: CustomerType.Premium }, _) => 0.2m,
        (_, { Hour: > 15 and < 20}) => 0.15m,
        ({ Age: < 25 }, _) => 0.1m,
        _ => 0m
    };
}

OK. That’s not as bad as the author’s version. It doesn’t allocate a tuple just to be able to use a tuple, for starters. But is it more legible than the previous version? Not at all. We could, of course, improve the formatting to align all of the return statements, but that’s also no fun to maintain.

The real issue with the pattern-matching solution is that we can no longer use local functions to improve semantics. The only thing we could do would be to add an IsStudent property directly to the class (extension properties are still being discussed (GitHub)). We cannot improve the semantics of the pattern-matching on DateTime because that type is not under our control.

In conclusion, as with anything else in programming, you should be judicious in where you use the new and shiny features, always considering whether they’re actually helping improve your code.