HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Checking if a Chess space has been attacked

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
spacecheckingbeenchesshasattacked

Problem

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.

Solution

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

Code Snippets

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;
}
else if(piece_abs_val==6){//king
        if(i==0){
            is_attacked=true;
        }
var Piece = {
    KNIGHT: 2,
    KING:   6,
    ...
}
if(piece_abs_val==Piece.KNIGHT){
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;
}

Context

StackExchange Code Review Q#114234, answer score: 9

Revisions (0)

No revisions yet.