Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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.

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

marked as duplicate by DKnight, Walter, gnat, MichaelT, ChrisF Feb 13 '13 at 21:09

This question has been asked before and already has an answer. If those answers do not fully address your question, please ask a new question.

3  
Consider adding polymorphism to Direction. return direction.doStuff(squareFrom); –  MichaelT Feb 13 '13 at 19:09
2  
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. –  GlenH7 Feb 13 '13 at 20:35
1  
    
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
show 1 more comment

2 Answers

up vote 4 down vote accepted

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.

share|improve this answer
add comment

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.

share|improve this answer
add comment

Not the answer you're looking for? Browse other questions tagged or ask your own question.