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.