2
votes

I was reading the Github repository about the Clean Code concepts applied to C#, and I was surprised that the pattern matching strategy was recommended for to avoid type checking (in addition https://github.com/thangchung/clean-code-dotnet#avoid-type-checking-part-1

I can understand when you don't really have the control over types defined in a third party, but other than that I think it might be definitely better to define an interface to process some actions.

The example used in that repository is:

// Bad: btw this example cannot even work, the methods are not defined in Object. Going to fork and PR.
public Path TravelToTexas(object vehicle)
{
    if (vehicle.GetType() == typeof(Bicycle)) 
    {
        vehicle.PeddleTo(new Location("texas"));
    } 
    else if (vehicle.GetType() == typeof(Car)) 
    {
        vehicle.DriveTo(new Location("texas"));
    }
}

// Good: parent class / interface
public Path TravelToTexas(Traveler vehicle)
{
    vehicle.TravelTo(new Location("texas"));
}

or

// Good: pattern matching
public Path TravelToTexas(object vehicle)
{
    if (vehicle is Bicycle bicycle)
    {
        bicycle.PeddleTo(new Location("texas"));
    } 
    else if (vehicle is Car car) 
    {
        car.DriveTo(new Location("texas"));
    }
}

Except that pattern matching switch / is translates into something equivalent to if / else if / else using the is operator (translated itself obj as TargetType != null) + some conditions (and the fact that you don't have to declare some variables upfront).

My question is there any optimization that I am not aware of when using the pattern matching switch / is cause otherwise I don't really see the point of recommending this strategy...?

And the extract take from: https://docs.microsoft.com/en-us/dotnet/csharp/pattern-matching#when-clauses-in-case-expressions

To illustrate these new idioms, let's work with structures that represent geometric shapes using pattern matching statements. You are probably familiar with building class hierarchies and creating virtual methods and overridden methods to customize object behavior based on the runtime type of the object.

Those techniques aren't possible for data that isn't structured in a class hierarchy. When data and methods are separate, you need other tools. The new pattern matching constructs enable cleaner syntax to examine data and manipulate control flow based on any condition of that data. You already write if statements and switch that test a variable's value. You write is statements that test a variable's type. Pattern matching adds new capabilities to those statements.

It's not possible for example to use an Adapter pattern instead and basically wrap the entities of the third party into something on which you can have the control. I mean then except in the deadly case to get an uninformative object I don't really see the point of doing this.

2

2 Answers

1
votes

(translated itself obj as TargetType != null)

Ok, that's not quite correct. Check the example on MSDN.

Old way:

public static double ComputeArea(object shape)
{
    if (shape is Square)
    {
        var s = (Square)shape;
        return s.Side * s.Side;
    } 
    else if (shape is Circle)
    {
        var c = (Circle)shape;
        return c.Radius * c.Radius * Math.PI;
    }
    // elided
    throw new ArgumentException(
        message: "shape is not a recognized shape",
        paramName: nameof(shape));
}

Note the explicit casts. Now the C# 7.0 version:

public static double ComputeAreaModernIs(object shape)
{
    if (shape is Square s)
        return s.Side * s.Side;
    else if (shape is Circle c)
        return c.Radius * c.Radius * Math.PI;
    else if (shape is Rectangle r)
        return r.Height * r.Length;
    // elided
    throw new ArgumentException(
        message: "shape is not a recognized shape",
        paramName: nameof(shape));
}

So what's changed? Well, the old way you had to 1) cast the object, and 2) use explicit casts in case of value types. That is, using as on a value type was an exception. Now, the is keyword is updated here to cast and assign a variable, but it also handles value types (not using as behind the scenes). This makes the code a bit more concise/easier to maintain. Or, in the words of MSDN

In this updated version, the is expression both tests the variable and assigns it to a new variable of the proper type. Also, notice that this version includes the Rectangle type, which is a struct. The new is expression works with value types as well as reference types.

Perhaps only a small benefit, but the MSDN documentation points out the new object is limited in scope, based on the if statement. You were probably already doing this before (casting inside an if block), but now it's automatic.

0
votes

The answer is yes. Using pattern matching introduces a cost in terms of boxing. Sure, the new pattern matching is visually pleasing, but the performance will indeed suffer. Boxing is a well-known performance bottleneck.

The performance problem of the pattern matching is described in detail here: https://github.com/dotnet/coreclr/issues/17670

Thus, this will indeed produce more performant code:

if (shape is Square)
{
    var s = (Square)shape;
    return s.Side * s.Side;
}

Than this:

if (shape is Square s) // Introduces boxing :(
{
    return s.Side * s.Side;
}

All in all, do NOT use pattern matching on HOT paths.