5
\$\begingroup\$

I am creating a game called Othello, the objective of the game is to close in your opponent's stones either horizontally, vertically or diagonally. All the stones that get closed in are then turned to your color. Win the game by having the most stones of your color.

Small example:

b = black
w = white
0 = empty space


0 0 b w
0 b w 0
0 w b 0
0 0 0 0

Placing a white stone (.w) will turn the 2 black stones to white.

0 .w w w
0 w w 0
0 w b 0
0 0 0 0

I have made a big mess while trying to find all the stones that need to be turned in a way that I made a method for every direction to find the stones. All found stones that are considered closed in are then added to a single ArrayList and then turned over by another method. Besides a few small details like coordinate difference and the counter that makes the coordinates move, the code is pretty alike. But I'm struggling to find a good way to refactor it in a decent structure.

private void checkLeft(Bord bord) {
        boolean lineIsClosedIn = false;
        ArrayList<Coordinate> squaresLeft = new ArrayList<>();
        for (int i = this.getPositie().getPosX() - 1; i >= 0; i--) {
            Coordinate co = new Coordinate(i, this.getPositie().getPosY());
            Square sq = bord.getSquareAtLoction(co);
            if (sq.isTaken()) {
                if (sq.getStone().getColor() == this.getStone().getColor()) {
                    lineIsClosedIn = true;
                } else {
                    squaresLeft.add(co);
                }

            }

        }
        if (lineIsClosedIn) {
            squaresToTurn.addAll(squaresLeft);
        }
    }

    private void checkRight(Bord bord) {
        boolean lineIsClosedIn = false;
        ArrayList<Coordinate> squaresLeft = new ArrayList<>();
        for (int i = (this.getPositie().getPosX() + 1); i <= bord.getBreedte() - 1; i++) {
            Coordinate co = new Coordinate(i, this.getPositie().getPosY());
            Square sq = bord.getSquareAtLoction(co);
            if (sq.isTaken()) {
                if (sq.getStone().getColor() == this.getStone().getColor()) {
                    lineIsClosedIn = true;
                } else {
                    squaresLeft.add(co);
                }

            }

        }
        if (lineIsClosedIn) {
            squaresToTurn.addAll(squaresLeft);
        }
    }

    private void checkUp(Bord bord) {
        boolean lineIsClosedIn = false;
        ArrayList<Coordinate> squaresLeft = new ArrayList<>();
        for (int i = this.getPositie().getPosY() - 1; i >= 0; i--) {
            Coordinate co = new Coordinate(this.getPositie().getPosX(), i);
            Square sq = bord.getSquareAtLoction(co);
            if (sq.isTaken()) {
                if (sq.getStone().getColor() == this.getStone().getColor()) {
                    lineIsClosedIn = true;
                } else {
                    squaresLeft.add(co);
                }

            }

        }
        if (lineIsClosedIn) {
            squaresToTurn.addAll(squaresLeft);
        }
    }

    private void checkDown(Bord bord) {
        boolean lineIsClosedIn = false;
        ArrayList<Coordinate> squaresLeft = new ArrayList<>();
        for (int i = this.getPositie().getPosY() + 1; i <= bord.getLengte() - 1; i++) {
            Coordinate co = new Coordinate(this.getPositie().getPosX(), i);
            Square sq = bord.getSquareAtLoction(co);
            if (sq.isTaken()) {
                if (sq.getStone().getColor() == this.getStone().getColor()) {
                    lineIsClosedIn = true;
                } else {
                    squaresLeft.add(co);
                }

            }

        }
        if (lineIsClosedIn) {
            squaresToTurn.addAll(squaresLeft);
        }
    }

    private void checkLeftUp(Bord bord) {

        boolean lineIsClosedIn = false;
        ArrayList<Coordinate> squaresLeft = new ArrayList<>();
        for (int i = this.getPositie().getPosY() - 1, y = this.getPositie().getPosX() - 1; i >= 0 && y >= 0 - 1; i--, y--) {
            Coordinate co = new Coordinate(i, this.getPositie().getPosY());
            Square sq = bord.getSquareAtLoction(co);
            if (sq.isTaken()) {
                if (sq.getStone().getColor() == this.getStone().getColor()) {
                    lineIsClosedIn = true;
                } else {
                    squaresLeft.add(co);
                }

            }

        }
        if (lineIsClosedIn) {
            squaresToTurn.addAll(squaresLeft);
        }

    }

    private void checkRightUp(Bord bord) {

        boolean lineIsClosedIn = false;
        ArrayList<Coordinate> squaresLeft = new ArrayList<>();
        for (int i = this.getPositie().getPosY() - 1, y = this.getPositie().getPosX() + 1; i >= 0 && y <= bord.getBreedte() - 1; i--, y++) {
            Coordinate co = new Coordinate(i, this.getPositie().getPosY());
            Square sq = bord.getSquareAtLoction(co);
            if (sq.isTaken()) {
                if (sq.getStone().getColor() == this.getStonen().getColor()) {
                    lineIsClosedIn = true;
                } else {
                    squaresLeft.add(co);
                }

            }

        }
        if (lineIsClosedIn) {
            squaresToTurn.addAll(squaresLeft);
        }

    }

    private void checkLeftDown(Bord bord) {

        boolean lineIsClosedIn = false;
        ArrayList<Coordinate> squaresLeft = new ArrayList<>();
        for (int i = this.getPositie().getPosY() + 1, y = this.getPositie().getPosX() - 1; i <= bord.getLengte() - 1 && y >= 0; i++, y--) {
            Coordinate co = new Coordinate(i, this.getPositie().getPosY());
            Square sq = bord.getSquareAtLoction(co);
            if (sq.isTaken()) {
                if (sq.getStone().getColor() == this.getStone().getColor()) {
                    lineIsClosedIn = true;
                } else {
                    squaresLeft.add(co);
                }

            }

        }
        if (lineIsClosedIn) {
            squaresToTurn.addAll(squaresLeft);
        }

    }

    private void checkRightDown(Bord bord) {

        boolean lineIsClosedIn = false;
        ArrayList<Coordinate> squaresLeft = new ArrayList<>();
        for (int i = this.getPositie().getPosY() + 1, y = this.getPositie().getPosX() + 1; i <= bord.getLengte() - 1 && y <= bord.getBreedte() - 1; i++, y++) {
            Coordinate co = new Coordinate(i, this.getPositie().getPosY());
            Square sq = bord.getSquareAtLoction(co);
            if (sq.isTaken()) {
                if (sq.getStone().getColor() == this.getSt().getColor()) {
                    lineIsClosedIn = true;
                } else {
                    squaresLeft.add(co);
                }

            }

        }
        if (lineIsClosedIn) {
            squaresToTurn.addAll(squaresLeft);
        }

    }
\$\endgroup\$

3 Answers 3

4
\$\begingroup\$

Inconsistencies

Some of your conventions are annoyingly inconsistent.

  • Coordinate vs. getPositie() vs. getSquareAtLoction(…).

    First, please choose either Dutch or English. If Dutch, choose either "Positie" or "Plaats" or "Coördineren". If English, choose either "Position" or "Location" (note spelling) or "Coordinates" (note plurality).

    If ambivalent, just use English everywhere. I recommend Coordinates, getCoordinates(), and getSquare(Coordinates c) ("at coordinates" does not need to be part of the method name).

  • ArrayList<Coordinate> squaresLeft = new ArrayList<>();

    What is the difference between "coordinates" and "squares"? I'm not sure that the distinction is necessary. If you do want two classes, then write either

    ArrayList<Coordinate> coordinatesLeft = new ArrayList<>();
    

    or

    ArrayList<Square> squaresLeft = new ArrayList<>();
    
  • In checkRightUp(), for example, your loop header is rather nasty:

    for (int i = this.getPositie().getPosY() - 1, y = this.getPositie().getPosX() + 1; i >= 0 && y <= bord.getBreedte() - 1; i--, y++)
    

    Why are you assigning the x-coordinate to a variable named y?

    Why are the iteration variables i and y? Sane naming would be x and y. Alternatively, col and row.

    Why check <= bord.getBreedte() - 1 instead of < bord.getBreedte()?

Suggested solution

The only thing that changes between your eight methods is the direction of the walk. So, let's define an Enum with those eight directions. Then, make an Iterator that walks from the starting coordinate in a specified direction until the edge of the board is reached.

import java.util.Iterator;
import java.util.NoSuchElementException;

public class BoardWalk implements Iterator<Square> {
    public enum Direction {
                        N ( 0, -1),
                NW(-1, -1),     NE(+1, -1),
        W (-1,  0),                     E (+1,  0),
                SW(-1, +1),     SE(+1, +1),
                        S ( 0, +1);


        public final int dx, dy;

        Direction(int deltaX, int deltaY) {
            this.dx = deltaX;
            this.dy = deltaY;
        }
    }

    private final Board board;
    private Coordinate coord;
    private final Direction dir;

    public BoardWalk(Board b, Coordinate start, Direction dir) {
        this.board = b;
        this.coord = start;
        this.dir = dir;
    }

    /**
     * Stops at the edge of the board.
     */
    @Override
    public boolean hasNext() {
        int x = this.coord.getX() + this.dir.dx,
            y = this.coord.getY() + this.dir.dy;
        return 0 <= x && x < this.board.getWidth() &&
               0 <= y && y < this.board.getHeight();
    }

    @Override
    public Square next() {
        if (!this.hasNext()) {
            throw new NoSuchElementException();
        }
        return this.board.getSquareAtCoordinate(this.coord = new Coordinate(
            this.coord.getX() + this.dir.dx,
            this.coord.getY() + this.dir.dy
        ));
    }

    @Override
    public void remove() {
        throw new UnsupportedOperationException();
    }
}

Then, it's easy to write a method that flips the chips in all eight directions.

private void doFlips() {
    ArrayList<Square> squaresToFlip = new ArrayList<Square>(board.getWidth());
    Coordinate play = …;
    for (BoardWalk.Direction dir : BoardWalk.Direction.values()) {
        squaresToFlip.clear();
        Iterator<Square> walk = new BoardWalk(board, play, dir);
        while (walk.hasNext()) {
            Square sq = walk.next();
            if (!sq.isTaken()) {
                break;
            } else if (sq.getStone().getColor() != ….getColor()) {
                for (Square captured : squaresToFlip) {
                    captured.flip();
                }
                break;
            }
            squaresToFlip.add(sq);
        }
    }
}
\$\endgroup\$
5
\$\begingroup\$
  • Correctness. The loop may turn too many stones. You should break the loop as soon as it encounters a square not taken by the other color. Otherwise, in the positions like

    W.b.w; Wwb; etc
    

    (W is the last move) the black stone gets reversed.

  • Refactoring. Make a helper

    private void checkInDirecton(Bord bord, int dx, int dy)
    

    It looks exactly like your check functions, except that loop variables are incremented as

    x += dx, y += dy
    

    Now call it as

    checkLeft(Board bord) {
        checkInDirection(bord, -1, 0);
    }
    

    etc.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ nice catch on the bug. But I don't think your refactoring would work out, as there are more differences than just the increments. \$\endgroup\$
    – tim
    Mar 28, 2015 at 21:11
  • \$\begingroup\$ Indeed, without the break i was getting more stones turned then i should have. \$\endgroup\$
    – Vahx
    Mar 29, 2015 at 6:59
3
\$\begingroup\$

In the future to avoid this, whenever you copy-paste code, try to immediately think of a way to do it better (it's easier to do it correctly the first time, than to find the small differences in 8 methods).

First of all, you could extract the code that is exactly the same (this will make it easier to find differences in the remaining code):

    private boolean addSquare(Board board, List<Coordinate> list, Coordinate coordinate) {
        Square sq = bord.getSquareAtLoction(coordinate);
        if (sq.isTaken()) {
            if (sq.getStone().getColor() == this.getStone().getColor()) {
                return true;
            } else {
                list.add(coordinate);
            }
        }
        return false;
    }

And then use it like this:

private void checkRight(Bord bord) {
    boolean lineIsClosedIn = false;
    ArrayList<Coordinate> squaresLeft = new ArrayList<>();
    for (int i = (this.getPositie().getPosX() + 1); i <= bord.getBreedte() - 1; i++) {
        Coordinate co = new Coordinate(i, this.getPositie().getPosY());
        lineIsClosedIn = lineIsClosedIn & addSquare(board, squaresLeft, co);

    }
    if (lineIsClosedIn) {
        squaresToTurn.addAll(squaresLeft);
    }
}

A naive approach to the actual refactoring would be to find all elements that differ in the methods. These are:

  • starting value of i
  • starting value of y
  • increment/decrement i
  • increment/decrement y
  • termination condition (based on i and y)
  • what position is used (x or y)

So a generalized method might look like this:

private void check(Bord bord, int startI, int startY, IntUnaryOperator incrementI, IntUnaryOperator incrementY, IntIntToBool termination, PositionChooser positionChooser) {
    boolean lineIsClosedIn = false;
    ArrayList<Coordinate> squaresLeft = new ArrayList<>();
    for (int i = startI, y = startY; termination.apply(i, y); i = incrementI.applyAsInt(i), y = incrementY.applyAsInt(y)) {
        Coordinate co = new Coordinate(i, positionChooser.apply(this.getPositie()));
        Square sq = bord.getSquareAtLoction(co);
        if (sq.isTaken()) {
            if (sq.getStone().getColor() == this.getSt().getColor()) {
                lineIsClosedIn = true;
            } else {
                squaresLeft.add(co);
            }

        }

    }
    if (lineIsClosedIn) {
        squaresToTurn.addAll(squaresLeft);
    }
}

interface IntIntToBool {
    boolean apply(int i, int y);
}

interface PositionChooser {
    int apply(Positie position);
}

And then use it like this:

private void checkLeftDown(Bord bord) {
    check(board, this.getPositie().getPosY() + 1, this.getPositie().getPosX() - 1, i -> i++, y -> y--, (i, y) -> i <= bord.getLengte() - 1 && y >= 0, position -> position.getPosY());
}

It's not the prettiest code in the world (and depending on what you use it for, this might cause performance problems because of the many functions calls, eg in an AI), but it should work.

You could also create two general methods, one that checks horizontal/vertical and one that checks diagonally. Those will have less differences, and thus would need less parameters than the generalized method I posted above.

Also, as your code is a bit complex, you should definitely add unit tests (either test the public overall check method, or check all the private check methods via reflection). This would have caught the bug vnp found, and it makes changing your code a lot easier (you just run the tests afterwards and can be sure that you didn't break anything).

\$\endgroup\$
1
  • \$\begingroup\$ Turning my code into something pretty isn't possible :D i'm aware that i should probably find a better design when it comes to the directions, a friend of mine suggested i use an enum to pass a direction into a single method that will then check in that direction. As for unit tests, i have some set up, but i'm not the best at writing unit test... attempting to school myself in that by reading "the art of unit testing" \$\endgroup\$
    – Vahx
    Mar 29, 2015 at 8:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.