Complex Conditionals in a Strategy Game

Question

I have been looking at this method for a long time and I have finally refactored it into something that I believe is much more readable. I am not totally happy with it though so I would like to get other opinions about it. There are a great many questions about complex if-else structures here on Code Review, so I am sorry to be adding to it. Hopefully this one will be interesting, at least.

This method is very important to my Game as a whole. It checks to make sure that a Job is valid for a particular Floor in the tower. It is called when the User or AI selects a job to add to the JobQueue, and also again when the JobQueue tries to start the job. The problem is that the construction rules are different for floors that are above ground compared to those that are below ground.

In the past, I had a switch statement for the jobType, and then an if-else statement inside of that to check if the floor was above or below ground. Then inside of that were the necessary cases for all of the jobTypes. The downsides to this were that some of the code was repeated, everything was happening inside one method, and it was not totally clear what was happening.

I have refactored this down to a method that calls two other methods. It always calls the first method, and then it calls one of two additional methods depending on whether the job rules are different for above and below ground for that particular job. I am primarily concerned with the clarity and readability of this code, especially whether or not there are enough comments. If there are better ways to do this I would love to hear that also.

-(BOOL) checkIfFloor:(int)floorNumber isValidForJob:(JobType)jobType {
    DTTowerFloor *floor = [self floorFromFloorList:floorNumber];

    if (![self checkEarlyReturnConditionsForFloor:floor jobType:jobType]) {
        return NO;
    }

    if ([self isJobSimpleJob:jobType]) {
        //these conditions are the same for above and below ground
        if (![self checkIfFloor:floor isValidForSimpleJob:jobType]) {
            return NO;
        }
    } else {
        //these conditions are different for above and below ground
        BOOL aboveGround = floor.floorNumber >= 0;
        if (![self checkIfFloor:floor isValidForJob:jobType aboveGround:aboveGround]) {
           return NO;
        }

    }

    return YES;
}
-(BOOL) checkEarlyReturnConditionsForFloor:(DTTowerFloor *)floor jobType:(JobType)jobType {

    //check these conditions first for an early return
    if (floor.isRevealed == NO) {
        return NO;
    }
    if ([floor alreadyHasJobOfType:jobType]) {
        return NO;
    }
    if (floor.floorState == FloorUnderAttack && jobType != JobTypeFightEnemy) {
        return NO;
    }
    if (jobType == JobTypeFightEnemy && (floor.enemies.count == 0 && !floor.enemyCave)) {
        return NO;
    }
    if (floor.floorState == FloorDestroyed) {
        if (!(jobType == JobTypeCleaningJob || jobType == JobTypeFightEnemy)) {
            return  NO;
        }
    }

    return YES;
}
-(BOOL) isJobSimpleJob:(JobType)jobType {
    return (!(jobType == LadderJob || jobType == BottomBuildJob || jobType == WallBuildJob));
}
-(BOOL) checkIfFloor:(DTTowerFloor *)floor isValidForSimpleJob:(JobType)jobType {

    switch (jobType) {

        //simple jobs
        case JobTypeMining:
            if (floor.floorBuildState & FloorHasLadder && (int)floor.groundBlocks.count > 0) {
                return YES;
            }
            break;
        case JobTypeHaulItem:
            if (floor.itemsNeedingHauling.count > 0 && (int)floor.groundBlocks.count <= 0) {
                return YES;
            }
            break;

        //building jobs
        case RoomBuildJob:
        {
            NSInteger requiredBuildings = (FloorHasBottom | FloorHasWalls);
            if ((floor.floorBuildState & requiredBuildings) == requiredBuildings && ((floor.floorBuildState & FloorHasRoom) == 0)) {
                return YES;
            }
            break;
        }
        case SuperiorWallBuildJob:
        {
            NSInteger requiredBuildings = (FloorHasBottom | FloorHasWalls);
            if ((floor.floorBuildState & requiredBuildings) == requiredBuildings) {
                return YES;
            }
            break;
        }

        //enemies and animals
        case JobTypeFightEnemy:
            if (floor.floorState == FloorUnderAttack || floor.floorState == FloorDestroyed) {
                return YES;
            }
            break;

        case JobTypeCleaningJob:
            if (floor.floorState == FloorDestroyed && floor.enemies.count == 0 && !floor.enemyCave) {
                return YES;
            }
            break;
        case JobTypeHaulAnimalToPasture:
        case JobTypeHaulAnimalToSlaughterhouse:
            if (floor.animals.count > 0) {
                return YES;
            }
            break;
        case JobTypeBreedAnimals:
            if (floor.animals.count > 1) {
                return YES;
            }
            break;

        default:
           break;
    }   

    return NO;
}
-(BOOL) checkIfFloor:(DTTowerFloor *)floor isValidForJob:(JobType)jobType aboveGround:(BOOL)aboveGround {

    switch (jobType) {

        //building jobs
        case LadderJob:
            if (aboveGround) {
                if (((floor.floorBuildState & FloorHasLadder) == 0) && (floor.floorBuildState & FloorHasBottom)) {
                    return YES;
                }
            } else {
                if ((floor.floorBuildState & FloorHasLadder) == 0) {
                    return YES;
                }
            }
            break;
        case BottomBuildJob:
            if (aboveGround) {
                NSNumber *oneFloorBelowNum = [NSNumber numberWithInt:floor.floorNumber - 1];
                DTTowerFloor *oneFloorBelow = [self.towerDict objectForKey:oneFloorBelowNum];
                if (((floor.floorBuildState & FloorHasBottom) == 0) && (int)floor.groundBlocks.count <= 0 && (oneFloorBelow.floorBuildState & FloorHasWalls)) {
                    return YES;
                }
            } else {
                NSInteger requiredBuildings = (FloorHasLadder | FloorHasWalls);
                if (((floor.floorBuildState & requiredBuildings) == requiredBuildings) && ((floor.floorBuildState & FloorHasBottom)== 0)) {
                    return YES;
                }
            }
            break;
        case WallBuildJob:
            if (aboveGround) {
                NSInteger requiredBuildings = (FloorHasBottom | FloorHasLadder);
                if (((floor.floorBuildState & requiredBuildings) == requiredBuildings) && ((floor.floorBuildState & FloorHasWalls) == 0)) {
                    return YES;
                }
            } else {
                if ((floor.floorBuildState & FloorHasLadder) && ((floor.floorBuildState & FloorHasWalls) == 0) && (int)floor.groundBlocks.count <= 0) {
                    return YES;
                }
            }
            break;

        default:
            break;
    }

    return NO;
}

I should note that when it comes to bitwise conditionals such as (floor.floorBuildState & FloorHasLadder) and (floor.floorBuildState & FloorHasWalls) == 0), my understanding is that you should not pass those back directly such as return (floor.floorBuildState & FloorHasLadder) but should instead return YES or NO, because they are not pure bools and can cause problems when passed around directly. If this is incorrect, please let me know.

Also if you see any cases that do not have the JobType at the start of the enum field name, I just have not changed all of them yet. I will definitely be getting around to that eventually.

Answer 1

I should note that when it comes to bitwise conditionals such as (floor.floorBuildState & FloorHasLadder) and (floor.floorBuildState & FloorHasWalls) == 0), my understanding is that you should not pass those back directly such as return (floor.floorBuildState & FloorHasLadder) but should instead return YES or NO, because they are not pure bools and can cause problems when passed around directly. If this is incorrect, please let me know.

In general, this is mostly correct.

Technically, 0 is false, and everything else should be regarded as true.

Suppose we have this example:

int x = 2;

if (x) {
    foo();
}

if (x == YES) {
    bar();
}

In this example, foo() will execute, but bar() will not. Now, for starters, this is why we don't write == YES.

But people are foolish, and will.

floor.floorBuildState & FloorHasLadder will only ever result in being equal to 1 or 0 if FloorHasLadder is equal to 1. And chances are, its actual value could change, plus for any other values in this enum, it simply won't work.

So we need to make sure we're returning 1 or 0 from a method that claims to return a BOOL. Bitwise operations make no such guarantee.

LOGICAL operators, however, do make that guarantee.

So in the example of (floor.floorBuildState & FloorHasWalls) == 0, we'd be returning the result of the logical == operator, this is guaranteed to be either 1 or 0, YES or NO. Returning this value is perfectly fine.

We could force a logical operator onto everything by appending an && YES to everything, which will force everything to evaluate to either YES or NO, an expected return value... but this seems more likely to confuse maintainers.

Moreover, sometimes doing the explicit return and sometimes relying on the == 0 return will show inconsistency, which might encourage a foolish maintainer to make a poor decision to "fix" something that's not broken for the sake of consistency.

So with all that said, I think it's probably best to stick with the explicit returns.

---

One other thing I do want to point out is the fact that when you return out of a switch statement, the break is unnecessary.

In the case of both of your switch statements, you're always entering an if condition based on a logical operator, or break-ing the switch down to a final return NO;. Here, we can simply just return the result of the logic operator, get rid of all the breaks, get rid of the return NO at the bottom, and set the default case to return NO;