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;
}
}
1 Answer 1
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 thanDIM
, but easier to read. Same goes forCoord
andpos
. - your
Player
fields are somewhat confusing. What dowhite
,turn
andwhiteChosen
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
?)