Rewriting an extensive validation method with duplication of code in different circumstances

I'm creating a chess engine and I'm currently in the stage of adding validation. I'm starting off with the major validationrule: checking if a piece can move to the given target destination.

I have setup a system that allows me to very fluently define how a piece can move and validation is done on these rules.

An example for a Pawn:

public class Pawn : Piece {
    public Pawn(Color color)
        : base(color) {
        Name = ChessPiece.Pawn;
        Worth = 1;
    }

    public override List GetPossibleMoves() {
        return new List
            {
                new Move
                    {
                        Amount = 1,
                        Direction = Direction.Forwards,
                    },
                new Move
                    {
                        Amount = 2,
                        Direction = Direction.Forwards,
                    }
            };
    }
}

Or a Rook:

public override List GetPossibleMoves() {
        return new List
            {
                new Move
                    {
                        Amount = 7,
                        Direction = Direction.Vertical,
                    },
                new Move
                    {
                        Amount = 7,
                        Direction = Direction.Horizontal,
                    },
            };
    }

My Tests to check the code:

```
[TestMethod]
public void AllowedPawnMoveValidatorWithValidInput() {
var chessboard = _testCaseProvider.GetChessboardForPawnMoves();
var validator = new AllowedMoveValidator();
// Test if a few pawns can move 1 spot ahead
foreach (var location in chessboard.ToList().Where(x => x.Piece is Pawn)) {
var endLocation = new Location {
X = location.X,
Y = location.Piece.Color == Color.White ? location.Y + 1 : location.Y - 1,
Piece = null
};

Assert.AreEqual(ValidationResult.Allowed

There are 33 if statements in your validation, all highly nested. There are lots of ways you could deal with that; what immediately comes to mind are helper functions. Don't be afraid to use helper functions, especially if you can give them a good, descriptive name.

For instance, you could replace this:

if (piece.Color == Color.White) {
          // lots of stuff
        } else if (piece.Color == Color.Black) {
          // lots of other stuff
        }

with this:

if (piece.Color == Color.White) {
          moveWhite();
        } else {
          moveBlack();
        }

and keep doing that for subdecisions.

Looking at moveWhite() and moveBlack(), it seems like we're duplicating effort; move(Color) would be better. This would help prevent problems where there's a bug in one branch but not in the other.

After looking at some of the nested ifs, it looks like a decision table might help, if not solve the whole problem. The idea is to make a (possibly multidimensional) array, and index into it to find the solution. So you could do something like decisionTable[White][Vertical][Forwards] and get back a function (or an object with a method named .decide()) that you could give start, end, and move, and it would tell you whether it validates.

Some nitpicks:

In the definition of a pawn, you have an amount, which is an integer. That seems like it will be problematic when it comes to pieces which can cross the board in a single move. You also have both a direction and an orientation for the move, which seems duplicative. Either get rid of one, or find a better name for one, so that it's clear why we need both.

Making forwards for white different from forwards for black seems odd. Why not make forwards a single direction that applies differently to each color? Conceptually, pawns move forwards whether they're white or black.

Orientation.BothForwardsAndDownwards is a bit wordy and not quite clear.

This:

Assert.AreEqual(ValidationResult.Forbidden,
                                                validator.Validate(chessboard,
                                                                    location,
                                                                    new Location {
                                                                        X = location.X,
                                                                        Y = location.Piece.Color == Color.White ? location.Y + 3 : location.Y - 3,
                                                                        Piece = null
                                                                    }
                                ));

seems like it could be better formatted like this:

Assert.AreEqual(ValidationResult.Forbidden,
                            validator.Validate(chessboard,
                                               location,
                                               new Location {
                                                 X = location.X,
                                                 Y = location.Piece.Color == Color.White 
                                                     ? location.Y + 3 
                                                     : location.Y - 3,
                                                 Piece = null
                                               })
                          );

I'm not entirely sure that's an idiomatic C# way of formatting it, but it doesn't run off the page as much.

StackExchange Code Review Q#35718, answer score: 5