Skip to main content
added 665 characters in body
Source Link
user33306
user33306

One thing I noticed, you've got the same loop in each branch of a conditional(if-else). It would be cleaner and easier to understand and maintain to have one loop with different conditionals in it.

for(auto& mask : victoryMasks) {
    if(grid.turn[0u] && (mask & grid.x) == mask) {
        return GridState::VictoryX;
    }
    elseif(!grid.turn[0u] && (mask & grid.o) == mask) {
         return GridState::VictoryO;
    }
}

Has another thought about maintaining the original logic and still improving cleanliness and maintenance. Put the loop into a function that returns a bool and takes a PlayerBitset as a parameter:

bool CheckForVictory(PlayerBitset player)
{
    for(auto& mask : victoryMasks) {
        if((mask & player) == mask) {
            return true;
        }
    }
    return false;   
}


if(grid.turn[0u]) {
    if(CheckForVictory(grid.x){
        return GridState::VictoryX;
    }
}
else {
    if(CheckForVictory(grid.o){
        return GridState::VictoryO;
    }
}

You can follow this same pattern anywhere you see major duplications of your code.

One thing I noticed, you've got the same loop in each branch of a conditional(if-else). It would be cleaner and easier to understand and maintain to have one loop with different conditionals in it.

for(auto& mask : victoryMasks) {
    if(grid.turn[0u] && (mask & grid.x) == mask) {
        return GridState::VictoryX;
    }
    elseif(!grid.turn[0u] && (mask & grid.o) == mask) {
         return GridState::VictoryO;
    }
}

One thing I noticed, you've got the same loop in each branch of a conditional(if-else). It would be cleaner and easier to understand and maintain to have one loop with different conditionals in it.

for(auto& mask : victoryMasks) {
    if(grid.turn[0u] && (mask & grid.x) == mask) {
        return GridState::VictoryX;
    }
    elseif(!grid.turn[0u] && (mask & grid.o) == mask) {
         return GridState::VictoryO;
    }
}

Has another thought about maintaining the original logic and still improving cleanliness and maintenance. Put the loop into a function that returns a bool and takes a PlayerBitset as a parameter:

bool CheckForVictory(PlayerBitset player)
{
    for(auto& mask : victoryMasks) {
        if((mask & player) == mask) {
            return true;
        }
    }
    return false;   
}


if(grid.turn[0u]) {
    if(CheckForVictory(grid.x){
        return GridState::VictoryX;
    }
}
else {
    if(CheckForVictory(grid.o){
        return GridState::VictoryO;
    }
}

You can follow this same pattern anywhere you see major duplications of your code.

added 18 characters in body
Source Link
user33306
user33306

One thing I noticed, you've got the same loop in each branch of a conditional(if-else). It would be cleaner and easier to understand and maintain to have one loop with different conditionals in it.

for(auto& mask : victoryMasks) {
    if(grid.turn[0u] && (mask & grid.x) == mask) {
        return GridState::VictoryX;
    }
    elseif(!grid.turn[0u] && (mask & grid.o) == mask) {
         return GridState::VictoryO;
    }
}

One thing I noticed, you've got the same loop in each branch of a conditional(if-else). It would be cleaner and easier to understand and maintain to have one loop with different conditionals in it.

for(auto& mask : victoryMasks) {
    if(grid.turn[0u] && (mask & grid.x) == mask) {
        return GridState::VictoryX;
    }
    elseif((mask & grid.o) == mask) {
         return GridState::VictoryO;
    }
}

One thing I noticed, you've got the same loop in each branch of a conditional(if-else). It would be cleaner and easier to understand and maintain to have one loop with different conditionals in it.

for(auto& mask : victoryMasks) {
    if(grid.turn[0u] && (mask & grid.x) == mask) {
        return GridState::VictoryX;
    }
    elseif(!grid.turn[0u] && (mask & grid.o) == mask) {
         return GridState::VictoryO;
    }
}
added 1 character in body
Source Link
user33306
user33306

One thing I noticed, you've got the same loop in each branch of a conditional(if-else). I It would be cleaner and easier to understand and maintain to have one loop with different conditionals in it.

for(auto& mask : victoryMasks) {
    if(grid.turn[0u] && (mask & grid.x) == mask) {
        return GridState::VictoryX;
    }
    elseif((mask & grid.o) == mask) {
         return GridState::VictoryO;
    }
}

One thing I noticed, you've got the same loop in each branch of a conditional(if-else). I would be cleaner and easier to understand and maintain to have one loop with different conditionals in it.

for(auto& mask : victoryMasks) {
    if(grid.turn[0u] && (mask & grid.x) == mask) {
        return GridState::VictoryX;
    }
    elseif((mask & grid.o) == mask) {
         return GridState::VictoryO;
    }
}

One thing I noticed, you've got the same loop in each branch of a conditional(if-else). It would be cleaner and easier to understand and maintain to have one loop with different conditionals in it.

for(auto& mask : victoryMasks) {
    if(grid.turn[0u] && (mask & grid.x) == mask) {
        return GridState::VictoryX;
    }
    elseif((mask & grid.o) == mask) {
         return GridState::VictoryO;
    }
}
Source Link
user33306
user33306
Loading