Return to Answer

added 66 characters in body

Source Link

edited Dec 23, 2019 at 13:02

5.3k
1
9
21

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

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 (board[elPos.y][elPos.x]boardItem == blackKing || board[elPos.y][elPos.x]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, whiteKingwhiteKing].includes(boardItem))
                            ].includes(board[elPos.y][elPos.x])) {
                            board[elPos.y][elPos.x]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;
    }
});

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++) {
                if (elPos.x == pieceMoves.moves[i].x && elPos.y == pieceMoves.moves[i].y) {
                    if (board[elPos.y][elPos.x] == blackKing || board[elPos.y][elPos.x] == 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(board[elPos.y][elPos.x])) {
                            board[elPos.y][elPos.x].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;
    }
});

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

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

Source Link

created Dec 23, 2019 at 12:53

RomanPerekhrest

5.3k
1
9
21

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.

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++) {
                if (elPos.x == pieceMoves.moves[i].x && elPos.y == pieceMoves.moves[i].y) {
                    if (board[elPos.y][elPos.x] == blackKing || board[elPos.y][elPos.x] == 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(board[elPos.y][elPos.x])) {
                            board[elPos.y][elPos.x].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;
    }
});