patternjavascriptMinor
Checking if a Chess space has been attacked
Viewed 0 times
spacecheckingbeenchesshasattacked
Problem
I have this list of at least seven
For more context, this is happening inside a loop (hence the
Also, when
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
Same exact code only much cleaner. I also did some refactoring. Take
In the switch,
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:
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:
This also removes the need for those annoying comments.
You should also do this for your
This is what your code now looks like:
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.