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;
}
});
````
1 Answer 1
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;
}
});
-
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– Thez2019年12月23日 13:36:59 +00:00Commented Dec 23, 2019 at 13:36
Explore related questions
See similar questions with these tags.