I am currently learning Java and working on a class assignment. We're supposed to make a "weird version" of Chess. Like in original chess, pieces can't move if there is a piece in their way, the obvious exception being the Horse, which can hop over all pieces except Kings.
I have an abstract class Piece that is inherited by all the piece types, each with their own rules of movement. Most of them have this movement restriction, and so I have defined the methods in this class:
public boolean freeWayHorizontally(int xO, int yO, int xD) {
//RIGHT
if (xO < xD) {
for (int x = xO + 1; x < xD; x++) {
CrazyPiece thereIsPiece = Simulador.checkIfTheresPiece(x, yO);
if (thereIsPiece != null){
return false;
}
}
//LEFT
} else if (xO > xD) {
for (int x = xO - 1; x > xD; x--) {
CrazyPiece thereIsPiece = Simulador.checkIfTheresPiece(x, yO);
if (thereIsPiece != null){
return false;
}
}
}
return true;
}
public boolean freeWayVertically(int xO, int yO, int yD) {
//UP
if (yO < yD) {
for (int y = yO + 1; y < yD; y++) {
CrazyPiece thereIsPiece = Simulador.checkIfTheresPiece(xO, y);
if (thereIsPiece != null){
return false;
}
}
//DOWN
} else if (yO > yD) {
for (int y = yO - 1; y > yD; y--) {
CrazyPiece thereIsPiece = Simulador.checkIfThereIsPiece(xO, y);
if (thereIsPiece != null){
return false;
}
}
}
return true;
}
thereIsPiece(int x, int y) is a function from the Chess Simulator class that, given a position on the board, returns the piece in that position.
As is obvious, these two receive the same parameters (origin coordinates and a destination coordinate, where one of the destination coordinates is one of the piece's origin coordinates), so the only thing that really changes is the way thereIsPiece() is called. EDIT: And because of this, they're marked as duplicates, and from what I've been told, that's very bad!
However, I can't seem to figure out a way to solve this problem using only one of these methods; ALSO AN EDIT: I've tried overloading it, but then it'd work only vertically or horizontally (may have done it wrong).
The thing is I need these to be done separately to implement the Horse's movement, that overrides these methods:
public boolean freeWayHorizontally(int xO, int yO, int xD) { //Overriden by the Horse class
//RIGHT
if (xO < xD) {
for (int x = xO + 1; x <= xD; x++) {
CrazyPiece thereIsPiece = Simulador.checkIfTheresPiece(x, yO);
if (thereIsPiece != null && thereIsPiece.isKing){
return false;
}
}
//LEFT
} else if (xO > xD) {
for (int x = xO - 1; x >= xD; x--) {
CrazyPiece thereIsPiece = Simulador.checkIfTheresPiece(x, yO);
if (thereIsPiece != null && thereIsPiece.isKing){
return false;
}
}
}
return true;
}
public boolean freeWayVertically(int xO, int yO, int yD) { //Overriden by the Horse class
//UP
if (yO < yD) {
for (int y = yO + 1; y <= yD; y++) {
CrazyPiece thereIsPiece = Simulador.checkIfTheresPiece(xO, y);
if (thereIsPiece != null && thereIsPiece.isKing){
return false;
}
}
//DOWN
} else if (yO > yD) {
for (int y = yO - 1; y >= yD; y--) {
CrazyPiece thereIsPiece = Simulador.checkIfThereIsPiece(xO, y);
if (thereIsPiece != null && thereIsPiece.isKing){
return false;
}
}
}
return true;
}
And then calls its own type of movement check, also defined in the Piece class:
public boolean freeWayL(int xO, int yO, int xD, int yD) {
boolean fH, fV;
//Horizontal -> Vertical
fH = this.freeWayHorizontally(xO, yO, xD);
if (fH) {
fV = this.freeWayVertically(xD, yO, yD);
if (fV) {
return true;
}
}
//Vertical -> Horizontal
fV = this.freeWayVertically(xO, yO, yD);
if (dV) {
fH = this.freeWayHorizontally(xO, yD, xD);
if (fH) {
return true;
}
}
return false;
}
What can I do to avoid all this duplication, or even to make these validations better?
The first issue with your code is that you have different method on Simulador class to check positions:
// When is checking horizontally
CrazyPiece thereIsPiece = Simulador.pegaPecaPorCoordenada(x, yO);
// When is checking vertically
CrazyPiece thereIsPiece = Simulador.checkIfThereIsPiece(xO, y);
I don't see a reason why this should not be a single method.
I can see 2 different point of improvement here:
As an example:
public boolean freeWayHorizontally(int xO, int yO, int xD) {
int destination = xD;
int origin = xO + 1;
if (xO > xD) {
destination = xO - 1;
origin = xD;
}
for (int x = origin; x < destination; x++) {
CrazyPiece thereIsPiece = Simulador.pegaPecaPorCoordenada(x, yO);
if (thereIsPiece != null){
return false;
}
}
return true;
}
When you have the origin is before the destination, you move from origin to destination stight forward.
When the destination is before the origin, you just swap and move from destination to origin.
I think you should just add a method:
private boolean checkPositionIsFree(int x, int y) {
return Simulador.pegaPecaPorCoordenada(x, y) != null;
}
Now you need to have one for the horizontal and one for the vertical, until you can't merge the 2 methods.
And then you can rewrite your method like so:
public boolean freeWayHorizontally(int xO, int yO, int xD) {
int destination = xD;
int origin = xO + 1;
if (xO > xD) {
destination = xO - 1;
origin = xD;
}
for (int x = origin; x < destination; x++) {
if (checkPositionIsFree(x, yO)){
return false;
}
}
return true;
}
For the Horse, you just @Override the checkPositionIsFree() method with the proper condition (adding the check on the king), and everything should work.
Update
To cover completely the horse case you can have a method that work on the input data:
@Override
public boolean freeWayHorizontally(int xO, int yO, int xD) {
return super.freeWayHorizontally(xO, yO, xD + 1);
}
In such a way you can avoid to rewrite all the code.
By the way, your code here have some typos, maybe you rewrite it. It's better to check those kind of things, and have working code (in your case) or the exact code is failing, to avoid lost vouluntiers time following the wrong bug.