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?
6
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.
1
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.