Validating user input for the game of draughts/checkers

Question

I'm trying to script a C# game of Draughts/Checkers. I'm at the point of taking user input and validating it. I first check the input of current x and y coords to see they are numbers, then check the coord pair against the board. Then repeat for target coords. I end up with a really long function that has repetition, but can't figure out how to refactor it down.

The code for this function is:

public static Board MovePiece(Board board, Player player)
        {
            bool CanContinue = false;
            bool validNumber = false;
            int number = 0;
            int xCurrent = 0;
            int yCurrent = 0;
            int xTarget = 0;
            int yTarget = 0;

            int validationStage = 0;

            while (!CanContinue)
            {
                validNumber = false;

                requestXCoord(validationStage);

                while (!validNumber)
                {
                    if (Int32.TryParse(Console.ReadLine(), out number))
                    {
                        xCurrent = number;
                        validNumber = true;
                    }
                    else
                    {
                        Console.WriteLine(Message.notValidNumber);
                        requestXCoord(validationStage);
                        validNumber = false;
                    }
                }

                validNumber = false;

                requestYCoord(validationStage);

                while (!validNumber)
                {
                    if (Int32.TryParse(Console.ReadLine(), out number))
                    {
                        yCurrent = number;
                        validNumber = true;
                    }
                    else
                    {
                        Console.WriteLine(Message.notValidNumber);
                        requestYCoord(validationStage);
                        validNumber = false;
                    }
                }

                if (validatePosition(board, player, xCurrent, yCurrent, validationStage))
                {
                    CanContinue = true;
                }
                else
                {
                    CanContinue = false;
                }
            }

            validationStage = 1;
            CanContinue = false;

            while (!CanContinue)
            {
                validNumber = false;

                requestXCoord(validationStage);

                while (!validNumber)
                {
                    if (Int32.TryParse(Console.ReadLine(), out number))
                    {
                        xTarget = number;
                        validNumber = true;
                    }
                    else
                    {
                        Console.WriteLine(Message.notValidNumber);
                        requestXCoord(validationStage);
                        validNumber = false;
                    }
                }

                validNumber = false;

                requestYCoord(validationStage);

                while (!validNumber)
                {
                    if (Int32.TryParse(Console.ReadLine(), out number))
                    {
                        yTarget = number;
                        validNumber = true;
                    }
                    else
                    {
                        Console.WriteLine(Message.notValidNumber);
                        requestYCoord(validationStage);
                        validNumber = false;
                    }
                }

                if (validatePosition(board, player, xTarget, yTarget, validationStage))
                {
                    CanContinue = true;
                }
                else
                {
                    CanContinue = false;
                }
            }

            PerformMove(board, player, xCurrent, yCurrent, xTarget, yTarget);

            return board;
        }

The other called functions are:

private static Board PerformMove(Board board, Player player, int xCurrent, int yCurrent, int xTarget, int yTarget)
        {
            if (canMakeMove(board, player, xCurrent, yCurrent, xTarget, yTarget))
            {
                switchOwnership(board, player, xCurrent, yCurrent, xTarget, yTarget);
                return board;
            }
            return null;
        }

        private static void switchOwnership(Board board, Player player, int xCurrent, int yCurrent, int xTarget, int yTarget)
        {
            board.tiles[xTarget, yTarget].OwnedBy = player;
            board.tiles[xCurrent, yCurrent].OwnedBy = null;
        }

        private static bool canMakeMove(Board board, Player player, int xCurrent, int yCurrent, int xTarget, int yTarget)
        {
            return board.tiles[xCurrent, yCurrent].OwnedBy == player
                            && board.tiles[xTarget, yTarget].OwnedBy == null;
        }

        private static void requestXCoord(int step)
        {
            var xCoord = step > 0 ? Message.targetX : Message.currentX;
            Console.Write(xCoord);
        }

        private static void requestYCoord(int step)
        {
            var yCoord = step > 0 ? Message.targetY : Message.currentY;
            Console.Write(yCoord);
        }

        private static bool validatePosition(Board board, Player player, int x, int y, int step)
        {
            if (x < 0 || y < 0 || x > 7 || y > 7)
            {
                Console.WriteLine(Message.outOfBounds);
                return false;
            }
            else if (board.tiles[x, y].OwnedBy == null && step == 0)
            {
                Console.WriteLine(Message.emptySpace);
                return false;
            }
            else if (board.tiles[x, y].OwnedBy == null && step == 1)
            {
                return true;
            }
            else if (board.tiles[x, y].OwnedBy == player && step == 1)
            {
                Console.WriteLine(Message.youOwnThis);
                return false;
            }
            else if (board.tiles[x, y].OwnedBy != player)
            {
                Console.WriteLine(Message.opponentOwns);
                return false;
            }

            return true;
        }

Answer 1

This method revolves around one big piece of code repeated four times: Trying to read a number until it succeeds. We can refactor that into a separate method, accepting a string as argument containing the prompt to the user:

private int GetNumber(string message)
{
    int result;
    Console.WriteLine(message);
    while (!int.TryParse(Console.ReadLine(), out result))
    {
        Console.WriteLine(Message.notValidNumber);
        Console.WriteLine(message);
    }
    return result;
}

The while loop for the first set of coordinates then becomes:

    int validationStage = 0;

    do
    {
        xCurrent = GetNumber(requestXCoord(validationStage));
        yCurrent = GetNumber(requestYCoord(validationStage));
    }
    while (!validatePosition(board, player, xTarget, yTarget, validationStage));

do while is a while loop where the breaking condition is evaluated at the end of the loop, instead of the start. This is useful in this situation because we know we will have to evaluate the loop body at least once.

ValidationStage

Using ints as flags like this is confusing. When reading the code, it isn't immediately clear what you mean. Enums are make for this purpose:

public enum Stage
{
    SelectCurrent,
    SelectTarget
}

However, at this point, the only difference the stage makes is the string in the prompt and the logic in the validation method. For clarity, it is better to use the string immediately (else we would have to figure out what requestXYCoord does) and split the validation method (since they check for different things). Especially when you're going to start adding more rules (like checking for legal moves, instead of just looking whether the space is free, or forced moves) you don't want these two validation steps in the same method.

public static Board MovePiece(Board board, Player player)
{
    int xCurrent = 0;
    int yCurrent = 0;
    int xTarget = 0;
    int yTarget = 0;

    do
    {
        xCurrent = GetNumber(Message.currentX);
        yCurrent = GetNumber(Message.currentY);
    }
    while (!IsValidCurrentPosition(board, player, xCurrent, yCurrent));

    do
    {
        xTarget = GetNumber(Message.targetX);
        yTarget = GetNumber(Message.targetY);

    }
    while (!IsValidTargetPosition(board, player, xTarget, yTarget));


    PerformMove(board, player, xCurrent, yCurrent, xTarget, yTarget);

    return board;
}

private int GetNumber(string message)
{
    int result;
    Console.WriteLine(message);
    while (!int.TryParse(Console.ReadLine(), out result))
    {
        Console.WriteLine(Message.notValidNumber);
        Console.WriteLine(message);
    }
    return result;
}

private static Board PerformMove(Board board, Player player, int xCurrent, int yCurrent, int xTarget, int yTarget)
{
    if (canMakeMove(board, player, xCurrent, yCurrent, xTarget, yTarget))
    {
        switchOwnership(board, player, xCurrent, yCurrent, xTarget, yTarget);
        return board;
    }
    return null;
}

private static void switchOwnership(Board board, Player player, int xCurrent, int yCurrent, int xTarget, int yTarget)
{
    board.tiles[xTarget, yTarget].OwnedBy = player;
    board.tiles[xCurrent, yCurrent].OwnedBy = null;
}

private static bool canMakeMove(Board board, Player player, int xCurrent, int yCurrent, int xTarget, int yTarget)
{
    return board.tiles[xCurrent, yCurrent].OwnedBy == player
                    && board.tiles[xTarget, yTarget].OwnedBy == null;
}

private static bool IsValidCurrentPosition(Board board, Player player, int x, int y)
{
    if (x < 0 || y < 0 || x > 7 || y > 7)
    {
        Console.WriteLine(Message.outOfBounds);
        return false;
    }
    else if (board.tiles[x, y].OwnedBy == null)
    {
        Console.WriteLine(Message.emptySpace);
        return false;
    }
    else if (board.tiles[x, y].OwnedBy != player)
    {
        Console.WriteLine(Message.opponentOwns);
        return false;
    }

    return true;
}

private static bool IsValidTargetPosition(Board board, Player player, int x, int y)
{
    if (x < 0 || y < 0 || x > 7 || y > 7)
    {
        Console.WriteLine(Message.outOfBounds);
        return false;
    }
    else if (board.tiles[x, y].OwnedBy == null && step == 1)
    {
        return true;
    }
    else if (board.tiles[x, y].OwnedBy == player && step == 1)
    {
        Console.WriteLine(Message.youOwnThis);
        return false;
    }
    else if (board.tiles[x, y].OwnedBy != player)
    {
        Console.WriteLine(Message.opponentOwns);
        return false;
    }

    return true;
}

Answer 2

Remove number of parameters

There are methods which have many parameters, you can group similar paramters to a type, and reduce the number of parameters. Ex: bellow method have 6 parameters which xCurrent, yCurrent are properties of a Point.

private static void switchOwnership(Board board, Player player, int
xCurrent, int yCurrent, int xTarget, int yTarget)

Then you can create a Point type, and reduce number of parameters:

public class Point
{
    public int x;
    public int y;
}

private static void switchOwnership(Board board, Player player, Point currentPoint, Point targetPoint)
{
    board.tiles[targetPoint.x, targetPoint.y].OwnedBy = player;
    board.tiles[currentPoint.x, currentPoint.y].OwnedBy = null;
}

private static Board PerformMove(Board board, Player player, Point currentPoint, Point targetPoint)
{
    if (canMakeMove(board, player, currentPoint, targetPoint))
    {
        switchOwnership(board, player, currentPoint, targetPoint);
        return board;
    }
    return null;
}

//.......

Your can refer this link for more details: https://www.refactoring.com/catalog/introduceParameterObject.html

DRY:

There are duplicated code from your MovePiece() method. You should try to group similar logic, and create sub method to re-use them.

Ex: there are 2 similar while loops while (!validNumber) and 2 similar while loops while (!CanContinue). Then you can create sub methods for there while loops to re-use it. Then your source code now is shorter and more readble:

public enum CoordType
{
    xCoord,
    yCoord
}

private void GetInputPosition(ref int pos, CoordType coordType)
{
    coordType == CoordType.xCoord ? requestXCoord(validationStage) : requestYCoord(validationStage);
    var validNumber = false;

    while (!validNumber)
    {
        int number;

        if (Int32.TryParse(Console.ReadLine(), out number))
        {
            pos = number;
            validNumber = true;
        }
        else
        {
            Console.WriteLine(Message.notValidNumber);
            coordType == CoordType.xCoord ? requestXCoord(validationStage) : requestYCoord(validationStage);
            validNumber = false;
        }
    }
}


private void GetCoord(ref Point point, int validationStage)
{
    int number = 0;
    var CanContinue = false;

    while (!CanContinue)
    {
        GetInputPosition(ref point.x, CoordType.xCoord);
        GetInputPosition(ref point.y, CoordType.yCoord);

        CanContinue = validatePosition(board, player, point.x, point.y, validationStage);
    }
}


public static Board MovePiece(Board board, Player player)
{
    Point currentPoint = new Point();
    Point targetPoint = new Point();
    GetCoord(ref currentPoint, 0);
    GetCoord(ref targetPoint, 1);

    PerformMove(board, player, currentPoint, targetPoint);

    return board;
}