9
\$\begingroup\$

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.

\$\endgroup\$
0
9
\$\begingroup\$

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;
}
\$\endgroup\$
2
  • \$\begingroup\$ 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); \$\endgroup\$ – ajax333221 Dec 17 '15 at 0:49
  • 1
    \$\begingroup\$ 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! \$\endgroup\$ – ajax333221 Dec 17 '15 at 0:51
4
\$\begingroup\$

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;
};
\$\endgroup\$
4
\$\begingroup\$

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.

\$\endgroup\$
4
  • \$\begingroup\$ 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. \$\endgroup\$ – ajax333221 Dec 17 '15 at 0:33
  • \$\begingroup\$ @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. \$\endgroup\$ – TheCoffeeCup Dec 17 '15 at 0:38
  • \$\begingroup\$ 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 \$\endgroup\$ – ajax333221 Dec 17 '15 at 0:40
  • \$\begingroup\$ @ajax333221 Good point, will edit my answer. \$\endgroup\$ – TheCoffeeCup Dec 17 '15 at 0:41
0
\$\begingroup\$

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.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – Toby Speight Jan 21 at 12:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.