Cleaning up Javascript event listener that manages game-loop

Question

As I added more features to my chess game, such as an endgame screen. I eventually ended up having a lot of duplicate code, and complex logic with lots of nested if statements in loops. The usual advice is to add more functions, but I cannot think of ways to reorganise this code such that each part has a highly specific role so that it can be turned into a function. I also could try to split this over multiple event listeners, but that may become too complicated.

If any extra pieces of code, or clarification about the question is needed, feel free to ask.

Here is the code:

let movedCastlesAndKings = {
    hasWKMoved: false,
    hasWKSCMoved: false,
    hasWQSCMoved: false,
    hasBKMoved: false,
    hasBKSCMoved: false,
    hasBQSCMoved: false
};
let castlingPerms = {
    WKSC: false,
    WQSC: false,
    BKSC: false,
    BQSC: false
};
let boardHistory = [];
let lastDoubledPawnMove, enpassantSquare;

let humanPlayer = "w";
let humanPieces = WP;

let AIPlayer = "b";
let AIPieces = BP;

let currentPlayer = humanPlayer;
let currentPieces = humanPieces;

let movesForThisPiece, clickPosition, currentPlayerKingPosition, isCurrentPlayerInCheck, promotedPawnPosition; 
let isCheckMate, hashIndex, isHighlightSquare, isHighlightPossibleMoves, hasClickedBefore, oppositePlayer, elementToEdit;
let legalMoves = generateLegalMoves(enpassantSquare, castlingPerms, initKingPosition, board, "w");
let lastMoveFromAndToSQ = {
    from: undefined,
    to: undefined
}
let threatningPieces = [];

function updateGameLoop(event) {
    clickPosition = getBoardCellFromClick(event);

    if (!hasClickedBefore) {
        if (clickPosition.x < 8 && clickPosition.y < 8) {
            boardItem = board[clickPosition.y][clickPosition.x];

            if (boardItem != " ") isHighlightSquare = true;

            if (currentPieces.includes(board[clickPosition.y][clickPosition.x])) {
                moves = generateMovesForThisPiece(legalMoves, clickPosition);
                isHighlightPossibleMoves = true;
            }

            hasClickedBefore = true;
        }
    } else {
        if (isHighlightPossibleMoves) {
            let moveItem, toSQ;

            for (move = 0; move < moves.length; move++) {
                moveItem = moves[move];
                toSQ = moveItem.to;

                if (toSQ.x == clickPosition.x && toSQ.y == clickPosition.y) {
                    board = moveItem.node;
                    switchSides();

                    currentPlayer == "w" ? oppositePlayer = "b" : oppositePlayer = "w";

                    promotedPawnPosition = findPromotedPawns(board, oppositePlayer);

                    if (promotedPawnPosition) {
                        if (oppositePlayer == "w" ) {
                            document.getElementById("whitePromotionSelection").style.display = "inline";
                        } else {
                            document.getElementById("blackPromotionSelection").style.display = "inline";
                        }

                        window.promotePawns = function (piece) {
                            board[promotedPawnPosition.y][promotedPawnPosition.x] = piece;) {
                                document.getElementById("whitePromotionSelection").style.display = "none";
                            } else {
                                document.getElementById("blackPromotionSelection").style.display = "none";
                            }

                            currentPlayerKingPosition = findKing(board, currentPlayer);
                            movedCastlesAndKings = updateMovedCastlesAndKings(movedCastlesAndKings, board);
                            enpassantSquare = getEnpassantSquare(moveItem.doublePawnMove, board, currentPlayer);
                            castlingPerms = castlingPermissions(movedCastlesAndKings, board);
                            hashIndex = makeHashTableIndex(castlingPerms, enpassantSquare, board, currentPlayer);
                            legalMoves = generateLegalMoves(enpassantSquare, castlingPerms, currentPlayerKingPosition, board, currentPlayer);

                            lastMoveFromAndToSQ.from = moveItem.from;
                            lastMoveFromAndToSQ.to = toSQ;
                            threatningPieces = getThreatningPieces(board, currentPlayer);
                            isCurrentPlayerInCheck = isSquareUnderAttack(currentPlayerKingPosition, board, currentPlayer);
                            isCheckMate = isCheckmate(legalMoves.length, isCurrentPlayerInCheck);

                            if (isCheckMate && currentPlayer == humanPlayer) {
                                document.getElementById("gameover").style.display = "inline";
                                document.removeEventListener('click', updateGameLoop); 
                            }

                            promotedPawnPosition = undefined;
                        }
                    } else {
                        currentPlayerKingPosition = findKing(board, currentPlayer);
                        movedCastlesAndKings = updateMovedCastlesAndKings(movedCastlesAndKings, board);
                        enpassantSquare = getEnpassantSquare(moveItem.doublePawnMove, board, currentPlayer);
                        castlingPerms = castlingPermissions(movedCastlesAndKings, board);
                        hashIndex = makeHashTableIndex(castlingPerms, enpassantSquare, board, currentPlayer);
                        legalMoves = generateLegalMoves(enpassantSquare, castlingPerms, currentPlayerKingPosition, board, currentPlayer);

                        lastMoveFromAndToSQ.from = moveItem.from;
                        lastMoveFromAndToSQ.to = toSQ;
                        threatningPieces = getThreatningPieces(board, currentPlayer);
                        isCurrentPlayerInCheck = isSquareUnderAttack(currentPlayerKingPosition, board, currentPlayer);
                        isCheckMate = isCheckmate(legalMoves.length, isCurrentPlayerInCheck);

                        if (isCheckMate && currentPlayer == humanPlayer) {
                            document.getElementById("gameover").style.display = "inline";
                            document.removeEventListener('click', updateGameLoop); 
                        }
                    }

                    break;
                }   
            }   
        }

        isHighlightPossibleMoves = false;
        isHighlightSquare = false;
        hasClickedBefore = false;
    }
}

````

Answer 1

You declare a lot of variables up front. It's a good idea to constrain a variable's scope as much as possible. If a variable is declared at some block level, make sure it's required at that level (like with hasClickedBefore, which needs to be persistent) - otherwise, it'll be easier to read the code if the variable is only declared and used in the inner block that it's used in. Having large numbers of separate variables declared at a certain point means that much more cognitive overhead required when reading the code later. To take one example, rather than:

let clickPosition;
function updateGameLoop(event) {
    clickPosition = getBoardCellFromClick(event);
    // do stuff with clickPosition

Consider instead, if clickPosition isn't being used elsewhere:

function updateGameLoop(event) {
    const clickPosition = getBoardCellFromClick(event);
    // do stuff with clickPosition

Another benefit of this is that it allows you to declare variables with const, which is far preferable to let whenever possible. (const does not permit reassignment, unlike let - so, when you declare a variable with const, you'll always be certain of what it refers to without having to search through all intervening lines to make sure it didn't get reassigned to something else somewhere)

Whenever you have a large number of ending brackets } at the end of a script, often that's a sign that things can be refactored to make things more readable. Here, instead of an if followed by a very long else, consider an if which returns at the end - that way, there's no need for a separate block for the else. So, this:

function updateGameLoop(event) {
    clickPosition = getBoardCellFromClick(event);
    if (!hasClickedBefore) {
        // handle first click
    } else {
        if (isHighlightPossibleMoves) {
            // lots and lots of code
        }

        isHighlightPossibleMoves = false;
        isHighlightSquare = false;
        hasClickedBefore = false;
    }
}

can turn into:

function updateGameLoop(event) {
    clickPosition = getBoardCellFromClick(event);
    if (!hasClickedBefore) {
        // handle first click
        return;
    }
    // these 3 variables aren't used elsewhere below, might as well reassign them now:
    isHighlightSquare = false;
    hasClickedBefore = false;
    if (!isHighlightPossibleMoves) {
        return;
    }
    isHighlightPossibleMoves = false;
    // lots and lots of code
    // or even better, a function call instead here
}

You do

boardItem = board[clickPosition.y][clickPosition.x];
if (boardItem != " ") isHighlightSquare = true;
if (currentPieces.includes(board[clickPosition.y][clickPosition.x])) {

Since boardItem has already retrieved the item at that position, might as well use that instead of looking it up again:

boardItem = board[clickPosition.y][clickPosition.x];
if (boardItem !== " ") isHighlightSquare = true;
if (currentPieces.includes(boardItem)) {

Since you're using ES6+ syntax (which is great!), you can replace the ugly manual iteration required by an ordinary for loop with for..of, it's much cleaner:

for (const moveItem of moves) {

But, since you're trying to find an item in the array which matches a condition, Array.prototype.find would be even better. (see below for full code)

The conditional operator probably shouldn't be abused as a replacement for if/else (the below will also throw the linting error no-unused-expressions):

currentPlayer == "w" ? oppositePlayer = "b" : oppositePlayer = "w";

You can use the conditional operator here, by putting oppositePlayer on the left:

oppositePlayer = currentPlayer === 'w' ? 'b' : 'w';

You could also consider making toggling between players easier by using a boolean instead, eg currentPlayerIsWhite = true.

(Remember to use strict equality with ===, not == - == behaves pretty strangely when comparing expressions of different types. Even if you happen not to be working with different types, the use of == will worry people that you may be doing so.)

You can save a reference to the selected whitePromotionSelection elements instead of selecting them twice to be more DRY. (const whitePromotionSelection = document.getElementById("whitePromotionSelection")). Or, even better - it seems likely that you have a typo there with board[promotedPawnPosition.y][promotedPawnPosition.x] = piece;) {, and that you want to set the piece on the board, then either change whitePromotionSelection or blackPromotionSelection, depending on who the active player is. If this is the case, then use the conditional operator upfront to identify which element needs to be displayed inline / hidden (see below).

Now, for the most important part: most of the code in the window.promotePawns and the next else block is identical. Put the identical parts into a function instead, and call that function twice instead of writing the code again. That part of the code handles checking the state of the game after a move is finished, so maybe call it handlePostMove. Putting it all together:

function updateGameLoop(event) {
    const clickPosition = getBoardCellFromClick(event);

    if (!hasClickedBefore) {
        if (clickPosition.x < 8 && clickPosition.y < 8) {
            const boardItem = board[clickPosition.y][clickPosition.x];
            if (boardItem !== " ") isHighlightSquare = true;
            if (currentPieces.includes(boardItem)) {
                moves = generateMovesForThisPiece(legalMoves, clickPosition);
                isHighlightPossibleMoves = true;
            }
            hasClickedBefore = true;
        }
        return;
    }
    isHighlightSquare = false;
    hasClickedBefore = false;
    if (!isHighlightPossibleMoves) {
        return;
    }
    isHighlightPossibleMoves = false;
    // Use `.find` here, instead of a `for` loop:
    const moveItem = moves.find((moveItem) => {
        const toSQ = moveItem.to;
        return toSQ.x === clickPosition.x && toSQ.y === clickPosition.y;
    });
    if (!moveItem) {
        return;
    }
    const toSQ = moveItem.to;
    board = moveItem.node;
    switchSides();

    oppositePlayer = currentPlayer === 'w' ? 'b' : 'w';
    promotedPawnPosition = findPromotedPawns(board, oppositePlayer);

    const handlePostMove = () => {
        currentPlayerKingPosition = findKing(board, currentPlayer);
        movedCastlesAndKings = updateMovedCastlesAndKings(movedCastlesAndKings, board);
        enpassantSquare = getEnpassantSquare(moveItem.doublePawnMove, board, currentPlayer);
        castlingPerms = castlingPermissions(movedCastlesAndKings, board);
        hashIndex = makeHashTableIndex(castlingPerms, enpassantSquare, board, currentPlayer);
        legalMoves = generateLegalMoves(enpassantSquare, castlingPerms, currentPlayerKingPosition, board, currentPlayer);

        lastMoveFromAndToSQ.from = moveItem.from;
        lastMoveFromAndToSQ.to = toSQ;
        threatningPieces = getThreatningPieces(board, currentPlayer);
        isCurrentPlayerInCheck = isSquareUnderAttack(currentPlayerKingPosition, board, currentPlayer);
        isCheckMate = isCheckmate(legalMoves.length, isCurrentPlayerInCheck);

        if (isCheckMate && currentPlayer === humanPlayer) {
            document.getElementById("gameover").style.display = "inline";
            document.removeEventListener('click', updateGameLoop);
        }
    };
    if (!promotedPawnPosition) {
        handlePostMove();
        return;
    }
    // promotedPawnPosition is true:
    const promotionSelection = document.getElementById(oppositePlayer === "w" ? "whitePromotionSelection" : 'blackPromotionSelection');
    promotionSelection.style.display = "inline";

    window.promotePawns = function (piece) {
        board[promotedPawnPosition.y][promotedPawnPosition.x] = piece;
        promotionSelection.style.display = "none";
        handlePostMove();
        promotedPawnPosition = undefined;
    };
}

You could also consider splitting up the larger chunks of the game loop into separate functions. A 70 line function, while better than a 96 line function, is still a bit smelly. Maybe make a function for the !hasClickedBefore block, and one for handling a found move item (everything below the .find), and one for handling a pawn promotion.