Board game valid move method always allows a move. Why is this happening?

Question

I'm making a Reversi board game in Java. In Reversi you can place a piece if only next to a piece of the opposite colour and if the row or column has a same colour piece at the end. My method works for the former but not for the latter. For example, type into the console 6,4 to place a white piece. This is fine. Then type 6,3. The black piece is placed despite there not being another black piece on the row or column. The method responsible for this is endPieceCheck(). I've been stuck on this problem for a long time. What is the problem? Thanks in advance.

The method below simply looks at if there is another piece of the opposite colour above the placed piece.

public void validMove() {

    if (grid[choice1 - 2][choice2 - 1] == 'w' && player == 'b' && grid[choice1 - 1][choice2 - 1] == '.') { // up
        endPieceCheck();
        if (confirmMove = true) {
            grid[choice1 - 1][choice2 - 1] = player;
            currentPlayer(); // Swaps turn.
            System.out.println("test up 2");
            boardLayout(); // Updates board.
            confirmMove = false;
        }
    }

This method is what isn't working. It places the piece regardless if there is another piece or not on the same col/row.

public void endPieceCheck() {
    /**
     * Checks to see if there is a piece that is the same colour of the piece being
     * placed. This method doesn't work. Place a 'w' piece at by typing 6,4. Then
     * place a 'b' piece by typing 6,3. The piece is placed even though there is no
     * other 'b' piece on that row. That shouldn't happen.
     */

    if (player == 'w' && grid[choice1 - 2][choice2 - 1] == 'b') { // up
        for (int i = choice1 - 1; i > 0; i--) {
            if (grid[i][choice2 - 1] == 'w') {
                confirmMove = true;
            } else {
                System.out.println("Not valid up");
            }
        }
    }

Answer 1

There are a few bugs here, and some bad development practices which lead to the specific bug that you refer to in the question.

The bug is partly in the code in the first version of your post (code removed in version 2 is needed to see the bug).

Given the case in the comment in the code (Playing (6,4) followed by (6,3)), in Version 2 of the question, after correcting the wrong assignment of confirmMove in the if statement in validMove (reported by @Hitobat):

confirmMove is set to true in endPieceCheck.

Then validMove calls boardLayout, which calls chooseMove, which calls validMove. But at this point, confirmMove is already true (the line that resets it to false hasn't been reached yet, and will never be reached), so the move is accepted.

This could be fixed by setting confirmMove to false at the start of validMove, but there are better ways to fix it.

Development Practices

This bug would have been avoided if confirmMove wasn't given such a broad scope. It would make more sense for it to be a return value of endPieceCheck.

Also, the fact that many methods do things that are not implied by (or related to) their names (they're counter-intuitively named) makes this bug more difficult to see. For example, boardLayout does not just lay out the board - calling it also inputs a move (the validates it, etc.). This is also an example or poor separation of concerns (which again makes bugs more difficult to see). The principle is that different things, such as displaying the user interface, validating a move, and game logic, should be separate.

This recursion (methods calling themselves, indirectly in this case) also means that the stack (the internal structure that holds the list of methods that have been called and haven't returned) keeps growing, and in theory (if you played it for long enough), it would eventually run out of memory. (If you use recursion, there has to be a case in which the method does not recurse, but recursion does not make sense in this case.) What you need here is a simple loop that calls the methods to input a move, validate it, update the board, and display the board (instead of each of these calling the next one and one of them calling the first).

Other Bugs

It doesn't check the first row or column for a same-colour piece (the loop in endPieceCheck tests for > 0, but 0 should be included (>= 0) ).

It throws exceptions when given an X or Y coordinate of 0. (Because it subtracts 1 to try to check the adjacent cell.)

It doesn't check for a same-colour piece in diagonal directions.

It checks for a same-colour piece only in the first direction that an opposite-colour piece is found. (e.g. if you try to place a white piece, and there is there a black one directly above, but no other white piece in that direction, it rejects it, even if there is also a black piece directly below and another white piece below that). (validMove should test all directions until a valid one is found or all have been tried.)

When checking for a piece of the same colour as the one being played, it would accept a piece in such a place that there is a gap (empty cell) between it and the one being placed. (The loop in endPieceCheck should stop if an empty cell is reached.)

It continues inputting moves when the board is full. (It recurses indefinitely. It should loop until the board is full.)

Other Development Practices

Copy-and-paste coding: There is a lot of code copied verbatim to multiple places. This is bad because (apart from making the program longer) any changes that have to be made to it in future have to be made multiple times (and it's easy to forget to change all copies, or a developer might not know about the copies). It also makes the code more difficult / time-consuming to understand because a maintenance developer has to read all copies to check that they are the same.

This can be avoided by putting the copied code into a method and calling it in each place where there would otherwise be a copy; or by structuring the code so that the copied code is just not within the 'if' statement, for example.

Copy-and-modify coding: This is like Copy-and-paste, except that some part of what is copied is changed in each copy. (e.g. the code copied for each direction.)

This can be avoided by calling a method with different parameters for each place that it is needed. (See the prototype method below for how the parameters could be passed.)

Constants: The number 8, referring to the width and height of the board, is used in a lot of places. Good development practice would be to define these as constants. This not only means that they can easily be changed, but also makes it easier to understand the code (when you see BoardWidth, you know why it is 8).

protected final static int BoardWidth  = 8;
protected final static int BoardHeight = 8;

Parameters: Someone looking at just endPieceCheck can't tell that choice1 and choice2 are actually the Y and X coordinates minus 1. These could be given descriptive names, and converted to the more intuitive values (subtracting 1) just after inputting them. (It makes the code easier to understand.)

Applying these changes, the prototype of endPieceCheck could be:

/**
 * Tests whether there is a piece of the same colour as the one being placed in the given direction,
 * with no empty cells between it and the one being placed.
 * 
 * For diagonals, both xDiraction and yDirection are non-zero.
 * 
 * @param player  The colour of the piece being placed ('w' or 'b').
 * @param x       The x-coordinate of the piece being placed.
 * @param y       The y-coordinate of the piece being placed.
 * @param xDirection  -1 for left; 1 for right; 0 for vertical.
 * @param yDirection  -1 for up; 1 for down; 0 for horizontal.
 * @return  true iff there is a piece of the opposite colour in the given direction,
 *          making the placement of the piece at (x,y) valid.
 */
protected boolean endPieceCheck(char player, int x, int y, int xDirection, int yDirection)

Then you would only have to implement the check once (rather than 16 copies). Instead of i++ or i--, you would add xDirection and yDirection in each iteration of the loop.

Rather than having copies of the code for each colour being placed, you could check for the cell being the same or different to player and not empty.

You could call it like this, for checking the down and right diagonal direction:

boolean validMove = endPieceCheck(player, choice2-1, choice1-1, int 1, 1);

Testing for the adjacent piece (currently in validMove) could be similar. And you could use two loops (for xDirection and yDirection) to test each direction, explicitly checking for and ignoring the case where both are 0.