2
\$\begingroup\$

I am trying to refactor a chess game I am currently creating so the code is more flexible and maintainable, while doing this, I came across an event listener which I have no idea on how to make "cleaner". Some of the variable names in the event listener are unintuitive but apart from that, I would like to know how to structure the event listener such that it uses multiple methods. I have previously asked a question on Code Review which is also about refactoring, and contains the code from which this function came from, here is the link: Object-Oriented JavaScript chess game. If you have any questions or clarifications needed about the code, feel free to ask.

  document.addEventListener('click', function(event){
        if(!hasClicked){
          if(event.clientX < 480 && event.clientY < 480 && board[Math.floor(event.clientY / 60)][Math.floor(event.clientX / 60)] != "vacant"){
            if(humanPlayer.indexOf(board[Math.floor(event.clientY / 60)][Math.floor(event.clientX / 60)]) != -1){
              canMove = true;
              isHighlightPossibleMoves = true;
              hasClicked = true;
              highlightPos = {x: Math.floor(event.clientX / 60), y: Math.floor(event.clientY / 60)};
              pieceMoves = processMoves({x: Math.floor(event.clientX / 60), y: Math.floor(event.clientY / 60)}, board);
            } else {
              hasClicked = true;
              highlightPos = {x: Math.floor(event.clientX / 60), y: Math.floor(event.clientY / 60)};
              canMove = false;
            }
          }
        } else {
          if(canMove){
            advancePosition = {x: Math.floor(event.clientX / 60), y: Math.floor(event.clientY / 60)};
            for(i = 0; i < pieceMoves.moves.length; i++){
              if(advancePosition.x == pieceMoves.moves[i].x && advancePosition.y == pieceMoves.moves[i].y){
                if(board[highlightPos.y][highlightPos.x] == blackKing || board[highlightPos.y][highlightPos.x] == whiteKing){
                  if(pieceMoves.moves[i].x - 2 == highlightPos.x || pieceMoves.moves[i].x + 2 == highlightPos.x){
                    isCastling = true;
                  } else {
                    isCastling = false;
                  }
                }
                if(isCastling){
                  board = chess.returnCastledBoard({x: highlightPos.x, y: highlightPos.y}, pieceMoves.moves[i]);
                  chess = new Chess(board);
                  isCastling = false;
                } else {
                  if(
                    board[highlightPos.y][highlightPos.x] == whiteKingSideCastle ||
                    board[highlightPos.y][highlightPos.x] == whiteQueenSideCastle ||
                    board[highlightPos.y][highlightPos.x] == blackKingSideCastle ||
                    board[highlightPos.y][highlightPos.x] == blackQueenSideCastle ||
                    board[highlightPos.y][highlightPos.x] == blackKing ||
                    board[highlightPos.y][highlightPos.x] == whiteKing
                  ){
                    board[highlightPos.y][highlightPos.x].hasClicked = true;
                  }
                  board = chess.updateBoard(highlightPos, advancePosition);
                  chess = new Chess(board);
                  break;
                }
              }
            }
          }
          hasClicked = false;
          canMove = false;
          highlightPos = undefined;
          pieceMoves = undefined;
          advancePosition = undefined;
        }
      });

````
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Toward restructuring and optimization

The expressions Math.floor(event.clientX / 60) and Math.floor(event.clientY / 60) which represents element position are redundantly duplicated across a half of the entire posted function's content.
The Extract function technique is reasonably applied and expressed with a separate function:

function getElPosition(e){
    return {x: Math.floor(e.clientX / 60),
            y: Math.floor(e.clientY / 60)};
}

hasClicked and highlightPos variables are assigned with same values in both exclusive branches of if/else conditional.
Thus, the assignment statements are moved out as common ones.
Furthermore, both highlightPos and advancePosition are essentially point to the target event element position returned by mentioned getElPosition function. Therefore, they could be just eliminated (see the full approach below).


The condition:

if (pieceMoves.moves[i].x - 2 == highlightPos.x || pieceMoves.moves[i].x + 2 == highlightPos.x) {
    isCastling = true;
} else {
    isCastling = false;
}

is just a verbose version of:

isCastling = (pieceMoves.moves[i].x - 2 == elPos.x || pieceMoves.moves[i].x + 2 == elPos.x);

Noisy condition:

if (
    board[highlightPos.y][highlightPos.x] == whiteKingSideCastle ||
    board[highlightPos.y][highlightPos.x] == whiteQueenSideCastle ||
    board[highlightPos.y][highlightPos.x] == blackKingSideCastle ||
    board[highlightPos.y][highlightPos.x] == blackQueenSideCastle ||
    board[highlightPos.y][highlightPos.x] == blackKing ||
    board[highlightPos.y][highlightPos.x] == whiteKing
)

is replaced with flexible Array.includes feature:

if ([whiteKingSideCastle, whiteQueenSideCastle, blackKingSideCastle, 
     blackQueenSideCastle, blackKing, whiteKing].includes(board[elPos.y][elPos.x]))

The can_move flag can be eliminated and replaced with check of pieceMoves.length.

board[highlightPos.y][highlightPos.x] indexing is repeated multiple times and worth to be extracted into a variable:

let boardItem = board[elPos.y][elPos.x];

See the full optimized approach:

function getElPosition(e){
    return {x: Math.floor(event.clientX / 60),
            y: Math.floor(event.clientY / 60)};
}

document.addEventListener('click', function (event) {
    let elPos = getElPosition(event);
    if (!hasClicked) {
        if (event.clientX < 480 && event.clientY < 480 && board[elPos.y][elPos.x] != "vacant") {
            if (humanPlayer.indexOf(board[elPos.y][elPos.x]) != -1) {
                isHighlightPossibleMoves = true;
                pieceMoves = processMoves(Object.assign({}, elPos), board);
            }
            hasClicked = true;
        }
    } else {
        if (pieceMoves && pieceMoves.length) {
            for (let i = 0, len = pieceMoves.moves.length; i < len; i++) {
                let boardItem = board[elPos.y][elPos.x];
                if (elPos.x == pieceMoves.moves[i].x && elPos.y == pieceMoves.moves[i].y) {
                    if (boardItem == blackKing || boardItem == whiteKing) {
                        isCastling = (pieceMoves.moves[i].x - 2 == elPos.x || pieceMoves.moves[i].x + 2 == elPos.x);
                    }
                    if (!isCastling) {
                        if ([whiteKingSideCastle, whiteQueenSideCastle, blackKingSideCastle,
                            blackQueenSideCastle, blackKing, whiteKing].includes(boardItem))
                        {
                            boardItem.hasClicked = true;
                        }
                        board = chess.updateBoard(elPos, elPos);
                        chess = new Chess(board);
                        break;
                    }
                    board = chess.returnCastledBoard({x: elPos.x, y: elPos.y}, pieceMoves.moves[i]);
                    chess = new Chess(board);
                    isCastling = false;
                }
            }
        }
        hasClicked = false;
        pieceMoves = undefined;
    }
});
\$\endgroup\$
1
  • 1
    \$\begingroup\$ A function separated from the event listener with a name that's indicative of it's intent would be a good addition to this answer. \$\endgroup\$
    – Thez
    Commented Dec 23, 2019 at 13:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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