Is putting an assignment statement in an if statement bad practice? [closed]

Question

Based on the code snippets below (they have been shortened for clarity).

The purpose of the scoreBoardState method is to be used to determine a score for the state of the game at the leaf nodes in a minimax algorithm that will be passed up to determine the best move for the AI to make.

The hasThreeInARowAndTwoOpenSpaces_Horizontal is one of many similar methods that is called by scoreBoardState to determine whether some condition is met (such as a player having 3 token in a row). If it is true it returns the number of the player that fulfills that conditions then increases the score of that player (either the human player or the AI).

The method needs to be called in an if statement to check if the value returned isn't zero (which means some score should be added). I can either set the value returned by the method within the if statement (Which I did in the code snippet), or I can call the method again if it doesn't return 0 and then set it into the variable. Obviously the second method is less efficient, but it is more human readable and easier to notice what is happening.

The question is, is setting the variable returned by the method called within the if statement considered bad practice? Or is it ok since it is more efficient?

Note: The inefficiency of the second method grows fairly quickly since it is within a for-loop and this situation will arise many times as each condition is tested. It is also done for each leaf node in a minimax algorithm (each node can have 7 branches) means a depth of only 3 (the minimum I'm using) there are 343 leaf nodes andbuta depth of 7 (the highest I'm currently using) there are almost 825,000 leaf nodes.

/* scores the board state of a root node in a minimax algorithm
 * @gameState a 2 dimensional array that stores values for each space on the 
 * board. Stores 0 for empty or 1 or 2 if position is taken by a player
 */
int scoreBoardState (int[][] boardState) {

    int aiScore = 0;
    int playerScore = 0;

    int player = -1;

    for (int i = 0; i < boardState.length; i++) {
        for (int j = 0; j < boardState[i].length - 4; j++) {
            if (j < boardState[i].length - 5 && (player = hasThreeInARowAndTwoOpenSpaces_Horizontal(boardState, i, j)) != 0) {

                if (player == AI)   
                    aiScore += 1000; //magic number entered for clarity
                else if (player == PLAYER)
                    playerScore += 1000;

            }
            else if (i < boardState.length - 4 && j > 2 && (player = hasThreeInARowAndOneOpenSpace_Diagonal_UpperRightToLowerLeft(boardState, i, j)) != 0) {

                if (player == AI)   
                    aiScore += SCORE_THREE_IAR_ONE_OS;
                else if (player == PL)  
                    playerScore += SCORE_THREE_IAR_ONE_OS;
            }

        }

    }    

    return aiScore - playerScore;
}

/*
 * checks if, starting from the passed in coordinates, whether there are 3 
 * spaces taken by the same player with an empty space on either side in a horizontal direction (left to right).
 *
 * returns the player number if the result is true. returns 0 if the result 
 *is false or all spaces are empty
 */
int hasThreeInARowAndTwoOpenSpaces_Horizontal(int[][] boardState, int row, int col) {
    if (boardState[row][col] == 0
        && boardState[row][col + 1] == boardState[row][col + 2] && boardState[row][col + 2] == boardState[row][col + 3] 
        && boardState[row][col + 4] == 0) 
    {
        return boardState[row][col + 1];
    }

    return 0;
}

Answer 1

It certainly runs the risk of being unexpected by anybody reading the code, which makes the code more difficult to support. That's often a worthy thing to avoid.

In both cases if there is a performance cost to be avoided then you could modify the condition to become nested conditions. So instead of this:

if (j < boardState[i].length - 5 && (player = hasThreeInARowAndTwoOpenSpaces_Horizontal(boardState, i, j)) != 0) {

you might have something like this:

if (j < boardState[i].length - 5) {
    player = hasThreeInARowAndTwoOpenSpaces_Horizontal(boardState, i, j);
    if (player != 0) {

That way the performance penalty of the operation is still only incurred when it otherwise logically would be in the original code. But the existence of the operation, and its subsequent assignment to a local variable, becomes a lot more obvious. Anybody browsing the code will be able to immediately see what's going on with very little thought.

The benefit here is that the conditionals themselves are very clear and concise. Having long conditional comparisons can make code very difficult to follow, but a simple comparison is straightforward.

The drawback here is that you're creating nested conditionals. People tend not to like those. (Though in this case my personal opinion is that it's the much lesser of two evils.) But that can be addressed by refactoring the operations inside of each conditional into their own aptly-named methods, if the readability of that is preferred.