switch statement - Refactoring advice for big switches in C#

Question

I have an application in C#/Winforms that lets users place objects on a grid to create levels for a game. It has several tools for placing tiles/lights/doors/entities etc. Currently I just use an enum for storing the currently selected tool and have a switch statement to run each tools code. As I've been adding more tools to the application it's starting to get spaghetti like, with lots of duplicated code.

Here is a cutdown version of the mouse down function in my editor class:

    public void OnEditorViewMouseDown(Point mousePos)
    {
        // Check if the click is out of bounds.
        if (IsLocationOOB(mousePos)) return;

        if (CurrentTool == ToolType.GroundTile)
        {
            // Allow drags along whole tiles only.
            m_DragManager.DragType = DragManager.DragTypeEnum.Tile;
            m_DragManager.StartDrag(mousePos);
        }
        else if (CurrentTool == ToolType.WallTile)
        {
            // Allow drags along grid edges only.
            m_DragManager.DragType = DragManager.DragTypeEnum.Edge;
            m_DragManager.StartDrag(mousePos);
        }
        else if (CurrentTool == ToolType.PostTile)
        {
            // Allow drags along grid points only.
            m_DragManager.DragType = DragManager.DragTypeEnum.Point;
            m_DragManager.StartDrag(mousePos);
        }
        else if (CurrentTool == ToolType.AreaLight)
        {
            // Allow drags anywhere. ie. not snapped to the grid in some way.
            m_DragManager.DragType = DragManager.DragTypeEnum.FreeForm;
            m_DragManager.StartDrag(mousePos);
        }
        else if (CurrentTool == ToolType.PointLight)
        {
            m_CurrentWorld.AddLight(TranslateToWorldCoords(mousePos));
        }
        else if (CurrentTool == ToolType.PlaceEntity)
        {
            m_CurrentWorld.PlaceEntity(TranslateToWorldCoords(mousePos));
        }
    }

The switch is used in several other functions (OnMouseMove, OnMouseUp) and it seems like bad design (big switch copied in several functions). Any advice for refactoring something like this in a cleaner and more extensible way? I'm currently thinking of having a base Tool class and having each tool it's own class that overrides the functions it uses (OnMouseDown() etc.). Does this sound sensible?

Thanks for reading.

Answer 1

You have the good intuition.

Usually, in OOP, when you have rows of if's or humongous switches, it's a strong code smell. The best way to make this smell go away is to go with polymorphism.

You should go ahead with your idea, having a base abstract class BaseTool, with the different OnXXX methods implemented as nops (just returns, so you only have to specify the behavior if your tool knows how to act on the method), and have each tool inherit from BaseTool and implement its own behavior by overriding the relevant methods.

So your method ends up being

public void OnEditorViewMouseDown(Point mousePos)
{
  currentTool.OnEditorViewMouseDown(mousePos);
}

Depending on your design, you should also consider passing the DragManager to the method, so as not to be tied to instance variables laying around. An EditorContext (containing the DragManager) fitted with everything the method needs without having to fetch "global" variables would make your method more self-contained and less brittle when refactoring. The design itself will depend on the responsability: who is in charge of what.