Checking if a Chess space has been attacked

Question

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.

Answer 1

Switch your code up

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!

Your code is littered with magic numbers. These are bad.

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.

Judging from your comments, I was able to make this:

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;
}

Answer 2

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;
};

Answer 3

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.

Answer 4

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.