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

Use a real switch statement, it will most likely get translated into a single jmp in assembler.

Answer 2

I'm not terribly familiar with the syntax of C# but in general you could make the CurrentTool an object that has methods StartDrag, EndDrag which accept a DragManager as an argument. Then when the mouse is pressed down on the view you just invoke CurrentTool.StartDrag(m_DragManager).

Answer 3

Yea, you should absolutley have a base class (or at the very least an interface) that defines all the common methods needed across all Tools. Try to make this code work, and it'll give you a good idea of how to design your classes:

m_DragManager.DragType = CurrentTool.DragType;
m_DragManager.StartDrag(mousePos);

where "CurrentTool" is an instance of your base class or your interface.

So basically, when a "Tool" is selected, at that point you determine which derived Tool you're dealing with, but from that point on, you deal with the base class only, and forget about any enums or anything like that to determine the currently selected tool. Make sense?

Answer 4

http://www.refactoring.com/catalog/replaceConditionalWithPolymorphism.html

Answer 5

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.

Answer 6

Try flag enums. Each bit would be a different feature and would allow for better stacking of features per cell.

Then you could just check for each bit even in different methods.

Answer 7

Yes, polymorphism is what you want here. You should define either an abstract base class Tool or an interface ITool depending on if you need to add implementation to the base or not (i.e. if there there common functionality among all tools, you might use an abstract base class).

Your manager should then take a Tool or ITool when something needs to be done. Your tool will implement a Drag function that takes the information it needs and either return what it needs to return or do what it needs to do. Or you could implement an inversion of control between your Manager and your Tool and inject the Manager into the Tool via property injection (Tool.Manager = dragManager) and let the tool do what it needs to do using the manager.

Answer 8

Sounds like a good place to use the Strategy Pattern: http://www.google.com/search?q=c%23+strategy+pattern