0

I am working on an app with a designer, in which can resize some shape items. To resize different shapes in only one 'ResizeThumb' type, I have this two programs:

switch(Mode)
{
    case Mode.CircleCenter:
        if (item is Circle circle1)
        {
            // ...
        }
        break;

    case Mode.CircleRadius:
        if (item is Circle circle2)
        {
            // ...
        }
        break;

    case Mode.RectTopLeft:
        if (item is MyRect rect1)
        {
            // ...
        }
        break;

    // many cases...

    default: break;
}

or

if (item is Circle circle)
{
    switch (Mode) // just 2 cases
    {
        case Mode.CircleCenter:     
            // ...
            break;
        case Mode.CircleRadius:     
            // ...
            break;
        default: break;
    }       
}
else if (item is MyRect rect)
{
    switch (Mode)
    {
        case Mode.RectTopLeft:      
            // ...
            break;          
        // 8 cases of every border and corner
        default: break;
    }       
}
else if (item is MyEllipse ellipse)
{
    switch (Mode) { /* 8 cases of every border and corner */ }
}
else if (item is Line ellipse)
{
    switch (Mode) { /* 2 cases of every endpoint */ }
}
else
{
    // No Code
}

When I have many shapes to resize, which will be faster and more stable? It seems that the first one will be faster, but I'm not sure. And the second one will be simple and convenient (no need use rect1, rect2 , ... ,rect8). Which should be used in my app?

Chen
  • 33
  • 1
  • 5
  • 5
    "*Which should be used in my app?*" -- to start with, always pick the clearest and most maintainable. If you need to change it for performance reasons, then break out a profiler and *measure* your improvements. Don't pick an option just because it seems it might be faster. This also looks like UI code: everything in UI code is cheap compared to actually drawing the UI, and slow bits have to be *really* slow before anyone notices, so don't worry about it – canton7 May 10 '21 at 12:19
  • Withou considering the design, I found the second option more intuitive, more clean so more maintainable and more suitable. More OOP. Since [C# 7](https://learn.microsoft.com/dotnet/csharp/pattern-matching) you can [pattern-match](https://stackoverflow.com/questions/7252186/switch-case-on-type-c-sharp) all and put switches in a main switch. This is the third option... –  May 10 '21 at 12:21
  • Talk us through how you would get into `case Mode.CircleCenter:` and yet `item` is _not_ a `Circle`. – mjwills May 10 '21 at 12:22
  • 4
    My gut feeling is that you are doing this inside out. The item itself should be of type `IShape` and have a Resize method that takes a mode parameter. Then let the class take care of itself rather than having a huge switch. – mjwills May 10 '21 at 12:24

2 Answers2

2

Note that you can use the type pattern in the switch as well. So you don't actually need an if-else.

switch (item)
{
    case Circle circle:
        ...
    case MyRect rect:
        ...
    case MyEllipse ellipse:
        ...
    case Line line:
        ...
}

So you can nest switch statements. Whether the item or the mode is nested does not matter.


But another question arises here. Couldn't you make these actions virtual methods of the shapes? Let's assume that you have a base object

public abstract class Shape
{
    public abstract void ResizeThumb(Mode mode);
}

Then you would derive all the shapes from this base Shape

public class Circle : Shape
{
    public override void ResizeThumb(Mode mode)
    {
        switch(Mode)
        {
            case Mode.CircleCenter:
                //TODO: do circle things here
                break;
            case Mode.CircleRadius:
                //TODO: do circle things here
                break;
            case Mode.RectTopLeft:
                //TODO: do circle things here
                break;
            default:
                break;
        }
    }
}

Then you can call this method without caring about the type of the item

item.ResizeThumb(mode);

Also, I'm not sure, why resizing thumbs has to be done in a different way for different shapes. By applying a coordinate transformation, you can resize every shape the same way.

Olivier Jacot-Descombes
  • 104,806
  • 13
  • 138
  • 188
  • That's good now. I time the 3 methods and in the current situation (not too many cases ), the times is similar, but pattern-match is clearer. I want to resize rects and ellipses by borders and corners, while circles by center and radius, lines by two endpoints, so I need different ways to resize them. It makes my code complex, but more suitable to my teacher's requirements. – Chen May 10 '21 at 13:12
  • "_pattern-match is clearer_" No, don't use pattern matching if you can avoid it. By switching on type, your violating the open-closed principle. If you do this often, you will create an unmaintainable mess. – Johnathan Barclay May 10 '21 at 13:16
  • @JohnathanBarclay, this is not a problem of pattern matching in general. Long if-else chains and switch statements are often a hint that the code is more procedural than object-oriented and could be improved. That's what I suggest in the second part of my answer. – Olivier Jacot-Descombes May 10 '21 at 13:23
  • @OlivierJacot-Descombes Yes, I was responding to the comment from CHEN Sora. – Johnathan Barclay May 10 '21 at 13:26
  • @JohnathanBarclay quite the opposite. And the OCP isn't violated. What if the two classes are DTOs that have nothing to do with rendering? Why should the DTO know anything about thumbnails? What if they are `Boat` and `Car` in a GIS? Only the rendering layer would care about thumbnails. Why should the entities be polluted by another layer's *behavior*? – Panagiotis Kanavos May 10 '21 at 13:26
  • @JohnathanBarclay if pattern matching violated OCP programs in functional languages would be very brittle. And yet, it's the OOP programs that frequently need breaking changes to accomodate modifications. – Panagiotis Kanavos May 10 '21 at 13:27
  • Well. I need to move the resize code to every shapes, so I need to redesign my base class, and there will be no problem on 'switch'. Good advice. – Chen May 10 '21 at 13:32
  • Yes, it also makes adding new shapes easier. – Olivier Jacot-Descombes May 10 '21 at 13:35
  • @PanagiotisKanavos It absolutely violates the OCP. If `Mode` is a concept of the "rendering layer" then you're right that the shapes shouldn't be polluted with it. But if pattern matching is used, the author of the shapes layer could add a new shape type and break the rendering. – Johnathan Barclay May 10 '21 at 13:53
  • @PanagiotisKanavos Pattern matching has its uses, but if OP has control over all the code, there is almost always a better solution. Without seeing more code its's hard to advise further. – Johnathan Barclay May 10 '21 at 13:54
  • Pattern matching is good. When I didn't understand how to move the `resize` method to every shape, it did help. But in this app, OP is better to solve my problem. It can make the enum `Mode` have less elements, and the class `ResizeThumb` more simple. But OP may not solve all problems, I am very grateful that Olivier Jacot-Descombes told me about pattern matching. – Chen May 10 '21 at 14:10
1

My suspicion is that you should not be using a switch and you should not be testing the object type. This is a classic illustration of polymorphism in an object-oriented language. Your code should look like this.

namespace ConsoleApplication1
{
    abstract class Shape
    {
        abstract public void ResizeThumb();
    }

    class Circle : Shape
    {
        public override void ResizeThumb() {
            // Your code to resize a circular thumbnail goes here
        }
    }

    class Ellipse : Shape
    {
        public override void ResizeThumb() {
            // Your code resize an elliptical thumbnail goes here
        }
    }
    class Rectangle : Shape
    {
        public override void ResizeThumb() {
            // Your code to resize a rectangular thumbnail goes here
        }
    }

    class Program
    {
        public void Main() {

            // Create a collection of shapes.
            Shape[] shapes = new Shape[] {
                new Circle(),
                new Ellipse(),
                new Rectangle(),
            };

            // Resize each shape's thumbnail.
            foreach(Shape shape in shapes) {
                shape.ResizeThumb();
            }
        }
    }
}
Nicholas Hunter
  • 1,791
  • 1
  • 11
  • 14
  • The problem with this is it hard-codes the `ResizeThumb` behavior to an object that may not need it, eg a DTO or in general, an object that isn't concerned with UI rendering – Panagiotis Kanavos May 10 '21 at 13:21
  • I take your point but I don't think it is applicable in this case The ask is to resize shapes, not resize a miscellaneous collection of objects which may include some shapes – Nicholas Hunter May 10 '21 at 13:29
  • No, it's to resize *thumbnails* for shapes. In a diagramming application the entities that deal with the diagram itself and the relation between shapes have very different concerns from the *renderers* that display those shapes as eg PDF, HTML or on screen. Think of a UML model for example. Why would the `Class` entity have to deal with resizing thumbnails? What are those thumbnails for? Obviously not displaying the shapes themseles. – Panagiotis Kanavos May 10 '21 at 13:33
  • On your machine, the code that creates thumbnails in a folder is different from the code that displays them and both are completely separate from the File Explorer or Finder code that deals with the files (never mind the file system). – Panagiotis Kanavos May 10 '21 at 13:34