Is setting a task in the wrong if practice?

Question

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

The goal of the scoreBoardState method should be used to determine the state of the game on the leaf nodes in a minimax algorithm that will be passed to determine the best movement for the AI.

hasThreeInARowAndTwoOpenSpaces_Horizontal is one of many similar methods that scoreBoardState is scoreBoardState to determine if a condition is fulfilled (for example, a player with 3 tokens in a row). If true, it returns the number of the player who fulfills these conditions, and then increases the score of that player (either a human player or an AI).

This method should be called in the if statement to check if the return value is returning (this means you need to add some point). I can either set the value returned by the method in the if statement (which I did in the code snippet), or I can call the method again if it does not return 0, and then sets it to a variable. Obviously, the second method is less effective, but it is more readable and easier to notice what is happening.

The question is, does the variable returned by the method called inside the if statement be considered bad practice? Or is this normal as it is more efficient?

Note. The inefficiency of the second method grows rather quickly, because it is inside the for loop, and this situation will occur repeatedly as each condition is checked. This is also done for each leaf node in the minimax algorithm (each node can have 7 branches) means a depth of only 3 (the minimum I use) is 343 leaf nodes and a depth of 7 (the highest I currently use) is 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

Of course, any user reading the code runs the risk of being unexpected, making code maintenance difficult. This can often be avoided.

In both cases, if there is a performance cost that should be avoided, you can change the condition to become nested conditions. So instead:

 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) { 

Thus, the penalty for the operation still remains only if it would otherwise be logically in the source code. But the existence of the operation and its subsequent assignment to a local variable become much more obvious. Anyone who views the code will be able to immediately see what happens with very small thoughts.

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

The downside here is that you are creating nested conventions. People tend not to like it. (Although in this case my personal opinion is that this is a much smaller of two evils.) But this can be eliminated by refactoring the operations inside each conditional expression into their own label methods, if their readability is preferable.

Answer 2

I would not say that this is bad practice. While it is used correctly. In your case, the use is excellent, since there is a valid condition that must be met before the variable must be set. In the top method, where this or that option is, you can think about using the following, but this is just a personal thing:

 condition ? true value : false value 

Using if statements is okay if they are used with else statements to stop extra ones, if instructions are executed then everything is fine.