# Checking if a Chess space has been attacked

I have this list of at least seven else-if. When their condition is met, I change the same boolean is_attacked to true. This should be simple but I can't figure out in what way I can achieve the same in a more readable way.

if(request_is_attacked){
if(as_knight){
if(piece_abs_val==2){//knight
is_attacked=true;
}
}else if(piece_abs_val==6){//king
if(i==0){
is_attacked=true;
}
}else if(piece_abs_val==5){//queen
is_attacked=true;
}else if(piece_direction%2){
if(piece_abs_val==4){//rook
is_attacked=true;
}
}else if(piece_abs_val==3){//bishop
is_attacked=true;
}else if(i==0 && piece_abs_val==1){
if(piece_val!=-1){//w_pawn
if(piece_direction==4 || piece_direction==6){
is_attacked=true;
}
}else{//b_pawn
if(piece_direction==2 || piece_direction==8){
is_attacked=true;
}
}
}
}


For more context, this is happening inside a loop (hence the i), when the i is the first it means it is one square apart (i==0), so Pawns and Kings use this, but this information shouldn't matter much.

Also, when piece_direction%2 is true, we are moving in a + (that's why I check for Rooks), else it means we are in moving x direction, so I proceed to check for Bishops and Pawns.

We can make this much more readable with a switch statement.

switch(piece_abs_val) {
case 1:
if(piece_val != -1){ //w_pawn
if(piece_direction == 4 || piece_direction == 6){
is_attacked = true;
}
} else { //b_pawn
if(piece_direction == 2 || piece_direction == 8){
is_attacked = true;
}
}
break;
case 2:
is_attacked = as_knight;
break;
case 3:
is_attacked = true;
break;
case 4:
is_attacked = piece_direction % 2;
break;
case 5:
is_attacked = true;
break;
case 6:
is_attacked = i == 0;
break;
}


Same exact code only much cleaner. I also did some refactoring. Take 6 for example; it used to be this:

else if(piece_abs_val==6){//king
if(i==0){
is_attacked=true;
}


In the switch, piece_abs_val is checked for 6. Then, is_attacked is set to whatever i==0 evaluates to. However, instead of using a conditional, I just simply wrote the statement out in the variable setting.

### Magic!

What you need here is a makeshift enumerated type. In case you don't know, an enumerated type is basically a type that assigns plain texts words (not strings) to integer values. I say "makeshift" because JavaScript does not have native support for these values.

var Piece = {
KNIGHT: 2,
KING:   6,
...
}


You can fill in the rest. Then, when in need of checking the value of piece, don't use the number; use this "enum". For example:

if(piece_abs_val==Piece.KNIGHT){


This also removes the need for those annoying comments.

You should also do this for your piece_directions.

This is what your code now looks like:

var Piece = {
PAWN: {
BLACK: -1,
WHITE: 1
},
KNIGHT: 2,
BISHOP: 3,
ROOK:   4,
QUEEN:  5,
KING:   6
}

switch(piece_abs_val) {
case Piece.PAWN:
if(piece_val != Piece.PAWN.BLACK){
if(piece_direction == 4 || piece_direction == 6){
is_attacked = true;
}
} else {
if(piece_direction == 2 || piece_direction == 8){
is_attacked = true;
}
}
case Piece.KNIGHT:
is_attacked = as_knight
case Piece.BISHOP:
is_attacked = true;
case Piece.ROOK:
is_attacked = piece_direction % 2;
case Piece.QUEEN:
is_attacked = true;
case Piece.KING:
is_attacked = i == 0;
}

• there was a sneaky thing I did when I separated an if-else with the if inside to ensure that if the chain continued, I could assume piece_direction%2 was false from there on, but everything is fixed if you use for the bishop is_attacked = !(piece_direction % 2); – ajax333221 Dec 17 '15 at 0:49
• also for the rook one to return a boolean we might consider double negating it or an equality check, and the i == 0 on the pawn is missing, but overall this was what I was looking for! – ajax333221 Dec 17 '15 at 0:51

I would consider using a switch statement with an enum:

Looking something like this:

var PIECES = {
King: 6,
Queen: 5,
Rook: 4,
Bishop: 3,
Knight: 2,
Pawn: 1
}
switch(piece_abs_val){
case PIECES.King:
//do stuff here.
break;
case PIECES.Queen:
break;
case PIECES.Rook:
break;
case PIECES.Bishop:
break;
case PIECES.Knight:
break;
case PIECES.Pawn:
break;
default //this is the else in your version
break;
};


Use && and ||, as well as ternary operaters:

if (request_is_attacked && (
(as_knight && piece_abs_val == 2)           ||
(piece_abs_val == 6 && i == 0)              ||
(piece_abs_val == 5)                        ||
(piece_direction % 2 ?
(piece_abs_val == 4) :
((piece_abs_val == 3)               ||
((i == 0 && piece_abs_val == 1)     &&
((piece_val != -1) ?
(piece_direction == 4 || piece_direction == 6) :
(piece_direction == 2 || piece_direction == 8)))))) {
is_attacked = true;
}


This code ors all the possible cases, while still keeping it readable.

• you are missing a 6 here (piece_abs_val == && i == 0) and I think there are some extra ) or missing/missplaced (, also the (piece_abs_val == 3) (which is the bishop) is meant to only be true if piece_direction % 2 is false, but I understand your point/design, thanks. – ajax333221 Dec 17 '15 at 0:33
• @ajax333221 With the exception of the missing 6, and the missing (, the rest of the code does exactly what your original code does, from what I can see. – TheCoffeeCup Dec 17 '15 at 0:38
• not really, when I separate }else if(piece_direction%2){ and the if inside, its like a little trap to end the chain, if it continues I guarantee piece_direction%2 is false – ajax333221 Dec 17 '15 at 0:40
• @ajax333221 Good point, will edit my answer. – TheCoffeeCup Dec 17 '15 at 0:41

I eventually made things a little bit more Object-oriented and the code now looks like this:

if(as_knight){
if(square.isKnight){
rtn.isAttacked = true;
}
}else if(square.isKing){
if(!i){
rtn.isAttacked = true;
}
}else if(square.isQueen){
rtn.isAttacked=true;
}else if(piece_direction%2){
if(square.isRook){
rtn.isAttacked = true;
}
}else if(square.isBishop){
rtn.isAttacked=true;
}else if(!i && square.isPawn){
if(square.sign>0){
if(piece_direction===_DIRECTION_BOTTOM_RIGHT || piece_direction===_DIRECTION_BOTTOM_LEFT){
rtn.isAttacked = true;
}
}else{
if(piece_direction===_DIRECTION_TOP_RIGHT || piece_direction===_DIRECTION_TOP_LEFT){
rtn.isAttacked = true;
}
}
}


Still ugly but almost reads as english so I kind of like it this way.

• Thanks for posting this code. It's a good idea to summarise which changes you made, and why - a self-answer ought to review the code, just like any other answer. – Toby Speight Jan 21 at 12:00