Is it possible to avoid enormously big switch in that case?

Question

I'm writing a simple chess-related code with intention to write it clearly (performance doesn't matter at all). And this method I have doesn't look clean to me at all:

 public static Piece GetClosestPiece(Square squareFrom, Direction direction)
    {
        //TODO: rework this mess
        switch (direction)
        {
            case Direction.Top:
                return
                    _pieces.Where(p => p.Square.OnVerticalWith(squareFrom))
                           .OrderBy(p => p.Square.Y)
                           .First(p => p.Square.Y > squareFrom.Y);
            case Direction.Bottom:
                return
                    _pieces.Where(p => p.Square.OnVerticalWith(squareFrom))
                           .OrderByDescending(p => p.Square.Y)
                           .First(p => p.Square.Y < squareFrom.Y);
            case Direction.Right:
            <...>
            case Direction.TopLeft:
                return
                    _pieces.Where(p => p.Square.OnBackslashWith(squareFrom))
                           .OrderByDescending(p => p.Square.X)
                           .First(p => p.Square.X < squareFrom.X);
            default:
                throw new InvalidEnumArgumentException();
        }
    }

I store the pieces as list, so, here I'm using LINQ to find the piece closest to a given square on a given direction.

Do switch/if statements of this size (8 cases total here) always present a code smell?
Is there any way to refactor this part only, or should I consider reviewing the design of the whole solution?

Consider adding polymorphism to Direction. `return direction.doStuff(squareFrom);` — , Feb 13 '13 at 19:09
I wouldn't say that a moderately sized switch statement is always a code smell, especially when there is a natural limit to the number of cases (i.e., there are only ever 8 surrounding squares). — mcknz, Feb 13 '13 at 19:14
Both portions of this question are on-topic. The question about code smells is certainly appropriate for P.SE. The refactoring portion of the question could be written better to fit more easily within P.SE, but it's on-topic as well. — , Feb 13 '13 at 20:35
Refactoring Switch: [Refactoring : Bad Smells in Code : Switch Statements](http://sourcemaking.com/refactoring/switch-statements) Related material on c2: [SwitchStatementsSmell](http://c2.com/cgi/wiki?SwitchStatementsSmell). On SO [Ways to eliminate switch in code](http://stackoverflow.com/questions/126409/ways-to-eliminate-switch-in-code) and here on P.SE [Refactoring Switch Statements and is there any real use for Switch Statements at all?](http://programmers.stackexchange.com/questions/147214/refactoring-switch-statements-and-is-there-any-real-use-for-switch-statements-at) — , Feb 13 '13 at 20:55
Now that this has been marked as a duplicate, should it be migrated to CodeReview.SE so that the OP can get an answer? The question cited as a duplicate presents an abstract case, where this is a specific instance. — mcknz, Feb 13 '13 at 22:23
@mcknz Unfortunately, two questions where asked in a single question. The first question in the post is a duplicate of another. The second question should be asked as its own question (and maybe on CodeReview, maybe here - as GlenH7 pointed out, it is on topic here too). — , Feb 14 '13 at 03:20

score 4 · Accepted Answer · edited Feb 13 '13 at 20:47

1. Do switch/if statements of this size (8 cases total here) always present a code smell?

The size of the switch doesn't particularly matter, the number of times you have the switch does. If you are repeating yourself with this switch, it could or should indicate you need a better design perhaps using polymorphism. If your switch lives here and only here, then you might just live with it.

2. Is there any way to refactor this part only, or should I consider reviewing the design of the whole solution?

See point #1 about when to certainly consider a different approach. As for more immediate refactorings, at least consider extracting the bodies of the "case" code to delegate elsewhere.

case Direction.Top:
     return GetFromTop(_pieces);
case Direction.Bottom:
     return ...

Keep your one method concerned only with the switch, have other methods concerned with the appropriate filtering and ordering.

Also a good reading regarding the "polymorphism" part: http://sourcemaking.com/refactoring/replace-conditional-with-polymorphism — heltonbiker, Jul 25 '14 at 14:39

score 1 · Answer 2 · answered Feb 13 '13 at 19:36

Have a look at the typesafe enum pattern and the command pattern. Don't use a C# enum in that case, but an interface.

interface Direction {

    // implements a single case in your switch
    public Piece Closest(Collection<Piece> pieces);

}

So you just need to do:

Directions.ValueOf(someDirectionLiteral).Closest(_pieces)

The Directions.ValueOf call maps a literal (a string or whatever) to an instance of the Direction interface.

The code may not be valid C#, but you should get.

Is it possible to avoid enormously big switch in that case?

2 Answers2