6
\$\begingroup\$

I'm currently just working on a Chess game project to improve my object-oriented skills. It is currently only at the starting stages and there is quite a bit missing (such as error checking) but I thought before I go any further I should get some advice on if I am using correct practices and if there's anything I could be doing better.

Game.java

public class Game {

    public static void main(String [] args)
    {

        //Creates players and board
        Player p1 = new Player();
        Player p2 = new Player();
        Board board = new Board();

        board.displayBoard();

        //Return players command coordinates (current and target)
        Coord[] command = p1.getCommand();

        //Gets piece from supplied coordinates
        Piece piece = board.getPiece(command[0]);


        if (piece != null && piece.getColor() == p1.getColor())
        {
            board.movePiece(piece, command[1]);

        }

        board.displayBoard();
    }
}

Player.java

import java.util.Scanner;

public class Player {

    private boolean white;
    private boolean turn;
    static boolean whiteChosen = false;

    public Player()
    {
        if (whiteChosen)
            white = turn = false;
        else
            white = turn = whiteChosen = true;
    }

    public Coord[] getCommand()
    {
        Scanner input = new Scanner(System.in);
        char[] command;

        do 
        {
            System.out.print("Move: ");
            command=input.nextLine().toCharArray();
            System.out.println();
        } while (!validFormat(command));

        input.close();


        int x1, x2, y1, y2;
        Coord pos1, pos2;
        x1 = (int) command[0] - 97;
        x2 = (int) command[3] - 97;
        y1 = (int) (command[1] - '0' - 8) * -1;
        y2 = (int) (command[4] - '0' - 8) * -1;
        pos1 = new Coord(x1, y1);
        pos2 = new Coord(x2, y2);

        return new Coord[] {pos1, pos2};

    }

    boolean validFormat(char[] command)
    {
        return true;
    }

    public boolean getColor()
    {
        return this.white;
    }
}

Board.java

public class Board {

    private Piece[][] pieces;
    public final int DIM = 8;
    private final String[] backline = {"ROOK", "KNIGHT", "BISHOP", "QUEEN", "KING", "BISHOP", "BISHOP", "ROOK"};

    //Sets up the board
    public Board()
    {
        pieces = new Piece[8][8];

        for (int y = 0; y < DIM; y++)
        {
            for (int x = 0; x < DIM; x++)
            {
                if (y == 0 || y == 7)
                    pieces[x][y] = new Piece(backline[x], new int[] {x, y}, true);

                else if (y == 1 || y == 6)
                    pieces[x][y] = new Piece("PAWN", new int[] {x, y}, true);
            }
        }
    }

    //Returns piece at given coordinates
    public Piece getPiece(Coord pos)
    {
        return pieces[pos.getX()][pos.getY()];
    }

    //Moves piece to given coordinates
    public void movePiece(Piece piece, Coord pos)
    {
        this.pieces[piece.getPosition().getX()][piece.getPosition().getY()] = null;

        this.pieces[pos.getX()][pos.getY()] = piece.setPosition(pos);
    }

    public void displayBoard()
    {
        for (int y = 0; y < DIM; y++)
        {
            for (int x = 0; x < DIM; x++)
            {
                if (pieces[x][y] != null)
                    System.out.print(pieces[x][y].getType() + " ");
                else
                    System.out.print("NULL ");
            }
            System.out.println();
        }
    }

}

Piece.java

public class Piece {

    private String type;
    private Coord position;
    private boolean white;
    private boolean unmoved;

    //Initializes a piece.
    public Piece(String type, int[] position, boolean white)
    {
        this.type = type;
        this.position = new Coord(position[0], position[1]);
        this.white = white;
        this.unmoved = true;
    }

    public Piece setPosition(Coord pos)
    {
        this.position = pos;
        return this;
    }

    public String getType()
    {
        return this.type;
    }

    public Coord getPosition()
    {
        return this.position;
    }

    public boolean getColor()
    {
        return this.white;
    }



}

Coord.java

public class Coord {

    private int x;
    private int y;

    public Coord(int x, int y)
    {
        setPosition(x, y);
    }

    public void setPosition(int x, int y)
    {
        this.x = x;
        this.y = y;
    }

    public int getX()
    {
        return this.x;
    }

    public int getY()
    {
        return this.y;
    }
}
\$\endgroup\$
4
\$\begingroup\$

OOP

OOP wise your code is pretty good.

I would change the piece type (and probably the color as well) from strings/boolean to enums, as they are easier to handle.

I would also separate the Board model and logic from the actual displaying of the board (you can leave your current method though, rename it to toString, and use it for debugging purposes), the advantage will be that you can easily switch how you present the board if you separate these concerns (ideally, you create interfaces). I would also extract the code to get player input from the model of the player (again, with interfaces), so that you can easily switch how you get input.

Your Piece class is a little odd, in that it accepts int[] position in the constructor, and then Coord pos in the setter for the same field. Also, it is the only place that your setter returns the object itself.

Naming

  • shortening variable names generally hurts readability. DIMENSION isn't too much longer than DIM, but easier to read. Same goes for Coord and pos.
  • your Player fields are somewhat confusing. What do white, turn and whiteChosen mean? The naming doesn't make this all that clear.

Comments

Most of your comments did not tell me anything I did not already know. //Initializes a piece. for example is not very helpful.

For method level comments, it is ok to duplicate the information the code already gave me, but you should use JavaDoc style comments, and you should tell me a bit more about corner cases. For example, movePiece says: //Moves piece to given coordinates, but it doesn't tell me what happens if I pass a piece that doesn't exist, or a coordinate outside the field, etc. Another example: getPiece tells me that it Returns piece at given coordinates, but not eg when it returns null, what that means, etc.

Misc

  • in Java, it is customary to place curly brackets on the same line as the opening statements.
  • use curly brackets even for one line statements for improved readability and to avoid future bugs.
  • you should not have magic numbers in your code; extract these to static variables at class level. That way, it is clear which numbers actually belong together, it's easier to change, and the name will tell a reader what the number means (eg why 97?)
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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