Why does this do-while loop not function?

Question

I have made a game of battleships using 2D Arrays and need my do-while loop to work so if you place a ship where there already is one, you get asked to place the ship again. Below is the code that changes the array values from 0 to 1 to resemble a ships placement. It throws an IllegalArgumentException if you try place a ship where there is already a value of 1.

public int[][] changeMatrixValues(int i, int j, int k, int l) throws IllegalArgumentException {    // for a ship with dimensions k*l and starting grid[i][j]
    for (mRow = i; mRow < (i + k); mRow++) {
        for (mCol = j; mCol < (j + l); mCol++) {
                    if (mMatrix[mRow][mCol] == 0 && mMatrix[mRow][mCol] != 1)
                        mMatrix[mRow][mCol] = 1;
                    else
                        throw new IllegalArgumentException("Ship already in area");
        }
    }
    return mMatrix;
}

This is the code in a different class that prompts the user for where they want to place a certain type of ship. It picks up the IllegalArgumentException, however, the do-while loop does not work and if the user places a ship on top of another, they do not get another chance to place that specific ship and the game moves on to ask where you would like to place the next ship. If anyone can highlight why this do-while loop is not working that'd be great!

    private boolean keepPlacing;
    private void ship(String shipToPlace, Matrix matrix, int k, int l) {
    keepPlacing = true;
    do {
        try {
            System.out.println(shipToPlace);
            chooseGrid();            // enter co-ords of where you want to place ship
            matrix.changeMatrixValues(mRow, mCol, k, l);
            keepPlacing = false;
        } catch (IllegalArgumentException ie) {
            System.out.println(ie.getMessage());
        }
    } while (keepPlacing);

    matrix.printLabeledMatrix();
}

OK. Seems fine at first. Can you debug and see what's the value of `keepPlacing` in the `catch` clause? — MordechayS, Dec 26 '16 at 19:11
It's worth pointing out that `changeMatrixValues` doesn't have failure atomicity: it doesn't check that the entire "change" is valid before making it. This means that it could set one or more cells and then fail, leaving things in a corrupt state. — Andy Turner, Dec 26 '16 at 19:15
Also: `mMatrix[mRow][mCol] == 0 && mMatrix[mRow][mCol] != 1` is redundant, since `0 != 1`. — Andy Turner, Dec 26 '16 at 19:16
Also: this is a case of using exceptions to signal something that's not an exceptional condition. Why not just return a boolean from that method: true for successful, valid move; false if it's invalid. You don't need to return the member variable. — Andy Turner, Dec 26 '16 at 19:23
Thanks Andy Turner for pointing out my other issues. The loop works fine, I had just moved the initial keepPlacing to outside the method which the debugger showed as always set to false causing the loop to fail — Brian Clegg, Dec 26 '16 at 20:22

score 1 · Answer 1 · answered Dec 26 '16 at 20:55

This change solves the issue:

private void ship(String shipToPlace, Matrix matrix, int k, int l) {
    boolean keepPlacing = true;
    do {
        try {
            System.out.println(shipToPlace);
            chooseGrid();
            if(matrix.validate(mRow, mCol, k, l) == true) {
                matrix.changeMatrixValues(mRow, mCol, k, l);
                keepPlacing = false;
            }
        } catch (IllegalArgumentException ie) {
            System.out.println(ie.getMessage());
        }
    } while (keepPlacing);

    matrix.printLabeledMatrix();
}

where the validate(mRow, mCol, k, l) method is:

public boolean validate(int i, int j, int k, int l) {
        for (mRow = i; mRow < (i + k); mRow++) {
            for (mCol = j; mCol < (j + l); mCol++) {
                if (mMatrix[mRow][mCol] == 1) {
                    System.out.println("Oops, try again");
                    return false;
                }
            }
        }
        return true;
}

Thanks for the help!

score 0 · Answer 2 · answered Dec 26 '16 at 20:08

0

In method changeMatrixValues you set array values to 1 and if throws IllegalArgumentException you don't rollback them.

answered Dec 26 '16 at 20:08

Vitali Biryukov

1

While this is likely to be a bug in the code, it doesn't appear to answer the question of why the `keepPlacing` loop exits when the `IllegalArgumentException` is thrown. If you don't have an answer for the posed question this statement should be reserved for a comment. – Guildencrantz Dec 26 '16 at 20:34

Why does this do-while loop not function?

2 Answers2