2

There is new feature introduced in C# 7.1 called generic pattern matching. One of the examples I found looks like this:

void Attack(IWeapon weapon, IEnemy enemy)
{
    switch (weapon)
    {
        case Sword sword:
            // process sword attack
            break;
        case Bow bow:
            // process bow attack
            break;
    }
}

But from my perspective it's a piece of incorrect design, which violates the second SOLID principle(open-close). I can't even think of the case where this might be needed, from my understanding if you came to situation when you need such switch, then you are doing something very wrong. From other hand, if this feature was added to language, there have to be a strong reason of doing so. So the question is - when would you need this assuming you're not creating poor architecture.

KorsaR
  • 536
  • 1
  • 10
  • 26
  • How is this violating the open-close principle? – asaf92 Oct 17 '19 at 09:26
  • 1
    It violates it by forcing you to edit the code to support extra types of `IWeapon` (code isn't closed for modification). – NibblyPig Oct 17 '19 at 09:35
  • why not add the attack method to the IWeapon interface ? weapon.Attack(enemy). It will be more OOP – Tohm Oct 17 '19 at 09:36
  • Many times it's easier to illustrate thing with "bad" code. Many tutorial and course online take this stand point. For sake of simplicity, the example are not solid or use eval and things like that. It should what it can do. Not what you should do. – xdtTransform Oct 17 '19 at 09:44
  • 2
    @KorsaR this has nothing to do with OCP, nor does it violate it. The concrete weapon classes weren't modified- in fact they *don't* have to be modified each time you want to write a new type-based functions. And yes, there were *very* strong reasons to add pattern matching as well as other functional features. They *reduce* fragility, they *don't* force you to modify `Weapon`, they allow you to write code that the compiler itself can verify is working correctly. Without it, you'd have to depend on runtime validations and casts to get the correct weapon type – Panagiotis Kanavos Oct 17 '19 at 09:50
  • @KorsaR anyone using functional languages or just C# functional features would argue that *this* `Attack` method is better from an OCP standpoint than something that uses double dispatch or worse, forces every concrete class to change to add a new `Attack` – Panagiotis Kanavos Oct 17 '19 at 09:54
  • 1
    I can't possibly agree, to add new types you would have to modify the code, `IWeapon` is just a charade and the method is in reverse, it should be on the `IWeapon`. But checking `IWeapon` against concrete types is a direct violation of OCP. You cannot add any new types of `IWeapon` to the code without modifying it, making `IWeapon` useless. At least if it was checking against `ISwingableObject` or `IShootableObject` I would agree that it is somewhat OCP compliant, even if the design is poor. – NibblyPig Oct 17 '19 at 09:58
  • @NibblyPig on the contrary, you *don't* have to modify any types just to add a new operation that *doesn't* concern weapons directly. Why should weapons know anything about enemies? What's the alternative anyway? Where would you put `Attack`? Where would you put rule changes that affect attack, eg player and enemy modifiers? In *every* weapon class? Why? They aren't *weapon* modifiers. Would you create different Attack types for every weapon and use double dispatch? Or have on `Attacks` method with a different override per weapon? – Panagiotis Kanavos Oct 17 '19 at 10:02
  • If I want to add `Spear`, `Knife`, `Dagger` classes that implement `IWeapon`, how do I do that without adding 3 more cases to the switch? You'd have to make them implement `Sword` if you wanted the same behaviour, which is silly. If this was a 3rd party library you'd be out of luck entirely. It should not be checking against concrete types like `Sword`. And as I said, it shouldn't be structured like this at all really, it should be using behaviours not concrete types to check for actions otherwise it becomes too rigid. – NibblyPig Oct 17 '19 at 10:07
  • @PanagiotisKanavos, and what if attack method is in third-party dll, where you don't have an access to the source code? How would you add a new weapon type then with this approach? – KorsaR Oct 17 '19 at 10:09
  • @NibblyPig if you wanted to add `TripUp` how would you do that? I can continue with the exact same comment you just wrote. – Panagiotis Kanavos Oct 17 '19 at 10:09
  • @KorsaR how would you do that without functional features? How would you retrieve properties that only exist in *some* types? If you pass a method or lambda you'll still be using functional features by the way - in fact, you can use LINQ as an example for your case: How can LINQ work when `Where` is implemented in a third-party library? – Panagiotis Kanavos Oct 17 '19 at 10:12
  • 1
    The idea behind OCP is that you can have multiple objects with the same operation and you're not constrained by having to implement the exact concrete type you want to perform the operation on. ie, you don't need to teach it sword/spear/knife, you only need to teach it the *operation*, i.e. `ISwingableWeapon`. If you want to add new operations then yes, you'd have to write new code (obviously). But if you're re-using the same operations, like in the classic Rectangle/Circle OCP explanation using *CalculateArea()*, then you make it OCP by using an interface instead of concrete types. – NibblyPig Oct 17 '19 at 10:14
  • @NibblyPig OK, what's the long range of an iron fist? Where would you put that in the interface? Interfaces aren't OCP, nor even a guarantee for OCP. They are just one feature that can help but you have to write the code. Pattern matching *significantly reduces* the effort needed to minimize changes. – Panagiotis Kanavos Oct 17 '19 at 10:16
  • 1
    Presumable that is defined in your `IronFist` class. `case ISwingableWeapon weapon: weapon.Swing();`. The ironfist class can take care of it. Then you can add literally any swingable weapon, like a `ChairLeg` and you never have to alter your class. – NibblyPig Oct 17 '19 at 10:17
  • But I don't have to do even that with pattern matching. Frankly, I suspect the only answer to this question is an Intro to functional programming in C#. I think Jon Skeet wrote one – Panagiotis Kanavos Oct 17 '19 at 10:18
  • 1
    To implelment `IronFist` into the above code, you would literally have to edit that class to put `case IronFist`, and so on, for every new weapon. Whereas if you did `case ISwingableWeapon` you'd never have to modify it again for *any* weapon of that type. – NibblyPig Oct 17 '19 at 10:18
  • @PanagiotisKanavos, I'm not sure I'm following the question. Common properties would be in interface, type-specific processing - in concrete Attack method implementation. That's how oop works – KorsaR Oct 17 '19 at 10:19

3 Answers3

1

A. A switch like this may be useful if you cannot (aka it's practically easier not to) modify classes you're working with, e.g. Exceptions.

B. pattern matching in switch can be much more sophisticated than just discriminating on types.

Pattern matching from What's new in C# 7.0 shows this versatility.

public static int SumPositiveNumbers(IEnumerable<object> sequence)
{
    int sum = 0;
    foreach (var i in sequence)
    {
        switch (i)
        {
            case 0:
                break;
            case IEnumerable<int> childSequence:
            {
                foreach(var item in childSequence)
                    sum += (item > 0) ? item : 0;
                break;
            }
            case int n when n > 0:
                sum += n;
                break;
            case null:
                throw new NullReferenceException("Null found in sequence");
            default:
                throw new InvalidOperationException("Unrecognized type");
        }
    }
    return sum;
}
tymtam
  • 31,798
  • 8
  • 86
  • 126
0

I agree that you can abuse it to violate open/closed, but you wouldn't use it in the way you have with your example (by taking in an interface).

Instead, consider a situation where you deserialize an object.

You might have a chat program and people can send messages, or images (pseudo code)

Object myObject = (object) _network.Read().Deserialize();

if (myObject is IImage) { }
else if (myObject is ITextMessage) { }

You could put this into a switch instead. It's still open closed because you can write additional types that extend IImage / ITextMessage.

You'd have to alter the fundamental functionality of the program to support things that aren't both of those. Or you could have an else { doSomeDefaultBehaviour(); }

NibblyPig
  • 51,118
  • 72
  • 200
  • 356
  • The example isn't an OCP violation at all. The concrete Weapon classes *weren't* modified just to add a new method. There was no need to add extra classes to implement double dispatching. In fact, the compiler itself can now verify the types are treated correctly. Without pattern matching and in C# 8, exhaustive switch expressions, you'd have to depend on runtime checks to do the same – Panagiotis Kanavos Oct 17 '19 at 09:53
  • It doesn't have anything to do with the Weapon classes being modified, it's to do with the code that takes in a type (usually an interface). You shouldn't have to modify the method to add extra types to your `switch` statement, you should have an interface that encompasses them. Although an interface is present here it's a red herring because it's not actually being used as an interface. – NibblyPig Oct 17 '19 at 10:03
  • And now you don't have to. This isn't about OCP at all. It's a functional style of programming, one with *far greater restrictions* on modification than OOP. The types *aren't* modified, only the method. In the end you *have* to write something to add new functionality. With functional programming, that something doesn't have to be in the types themselves. It's actually far easier to separate concerns – Panagiotis Kanavos Oct 17 '19 at 10:04
  • I don't think I can explain it any further. The class is not open for extension as despite taking in an `IWeapon` you're limited to two types, and you cannot get *any* extra behaviour out of it without *modification*. If it was at least `ISwingableWeapon` for example, then it would be *open to extension* as you'd be able to create new implementations of `IWeapon` and use them *without any modification*. – NibblyPig Oct 17 '19 at 10:11
  • @NibblyPig, and again, let's say that code piece you provided is in black-box dll, how would you add new datatype processing? Isn't it better to have some IDeserializable interface with some PostProcessing method? – KorsaR Oct 17 '19 at 10:16
  • You're talking about adding new operations, brand new functionality. If you wanted to add new *operations* on the fly I'd probably use reflection to dynamically load compatible processing libraries. But we're not talking about adding new operations, we're just talking about ensuring we can use existing operations with any object type. See this example http://joelabrahamsson.com/a-simple-example-of-the-openclosed-principle/ , we're talking about making the `Area` property work with any type of object without having to specify concrete types. Not adding other methods like `Circumference` – NibblyPig Oct 17 '19 at 10:25
0

I agree that having switch statements all over your codebase is harder to maintain and don't scale well.

However, if Sword and Bow are not dependent on the class that contains the Attack method, then you have no other option. And there are many examples for this like in serialization (as NibblyPig mentioned), but also when building a presentation layer, a ViewModel etc...

If Sword and Bow don't have the responsibility of processing an attack (which is where SRP is relevant) and you have a specific class for this case then you'll have to switch over different types. You don't want Sword to have a method that deals with serialization and a method that deals with what color will the Sword be in one specific menu etc...

Sure, it's helpful if you can avoid this by creating smart interfaces that abstract away the type and just allow you to access the information you need but sometimes you have no other options.

Also, sometimes language features can help to deal with legacy code and are not an indication of recommended good practices.

asaf92
  • 1,557
  • 1
  • 19
  • 30