Job queue that performs actions

Question

A brief background:

I'm working on a game prototype for a simple strategy game. After I created a number of possible jobs I wanted to create something to manage them. This is the first time I've attempted something like this so code review would be helpful.

I created a set of methods that adds Jobs to a Job Queue and assigns them to Dwarves if they don't currently have a job. That way, the player can add a number of jobs, and as long as the job is valid, it will be added to the Queue.

First the relevant enum:

typedef enum {
    MiningJob = 0,
    LadderJob,
    BottomBuildJob,
    WallBuildJob,
    RoomBuildJob
}JobType;

Here is the main code for the Job Queue:

#pragma mark - Job Queue
-(void) addJobToQueueOfType:(int)jobType forFloor:(int)floorNumber {
    if ([self checkIfFloor:floorNumber isValidForJob:jobType]) {
        DTJob *tempJob = [[DTJob alloc]init];
        tempJob.jobType = jobType;
        tempJob.floorNumber = floorNumber;
        [_jobQueue insertObject:tempJob atIndex:0];
    }
}
-(BOOL) checkIfFloor:(int)floorNumber isValidForJob:(int)jobType {
    NSNumber *floorNum = [NSNumber numberWithInt:floorNumber];
    DTTowerFloor *tempFloor = [self.gameTower.towerDict objectForKey:floorNum];
    switch (jobType) {
        case MiningJob:
            if (tempFloor.hasLadder && tempFloor.isRevealed && tempFloor.blockPercentageRemaining > 0) {
                return YES;
            }
            break;
        case LadderJob:
            if (!tempFloor.hasLadder && tempFloor.isRevealed && self.currentCommonResources >= _ladderCost && !tempFloor.hasActiveWorker) {
                return YES;
            }
            break;
        case BottomBuildJob:
            if (tempFloor.isRevealed && tempFloor.hasLadder && tempFloor.blockPercentageRemaining <= 0 && !tempFloor.hasBottom && self.currentCommonResources >= _bottomCost && !tempFloor.hasActiveWorker) {
                return YES;
            }
            break;
        case WallBuildJob:
            if (tempFloor.isRevealed && tempFloor.hasBottom && !tempFloor.hasWalls && self.currentCommonResources >= _wallCost && !tempFloor.hasActiveWorker) {
                return YES;
            }
            break;
        case RoomBuildJob:
            if (tempFloor.isRevealed && tempFloor.hasBottom && tempFloor.hasWalls && self.currentCommonResources >= _roomCost && !tempFloor.hasActiveWorker) {
                return YES;
            }
            break;
        default:
            break;
    }
    return NO;
}
-(void) assignJob:(DTJob *)job toDwarf:(DTDwarf *)dwarf {
    switch (job.jobType) {
        case MiningJob:
            [self assignMiningJobToDwarf:dwarf forFloor:job.floorNumber];
            break;
        case LadderJob:
            [self assignLadderJobToDwarf:dwarf forFloor:job.floorNumber];
            break;
        case BottomBuildJob:
            [self assignBottomBuildJobToDwarf:dwarf forFloor:job.floorNumber];
            break;
        case WallBuildJob:
            [self assignWallBuildJobToDwarf:dwarf forFloor:job.floorNumber];
            break;
        case RoomBuildJob:
            [self assignRoomBuildJobToDwarf:dwarf forFloor:job.floorNumber];
            break;
        default:
            break;
    }
}
//this is called from the update loop
-(BOOL) isDwarfAvailable {
    for (DTDwarf *tempDwarf in self.dwarves) {
        if (!tempDwarf.hasJobAssigned && !tempDwarf.isMoving) {
            _nextDwarf = tempDwarf;
            return YES;
        }
    }
    return NO;
}

Here is the call from the Update loop which runs every 0.25 seconds according to the time step of the SKScene:

-(void) checkAssignJobToDwarfFromQueue {
    if ([self isDwarfAvailable] && _jobQueue.count > 0) {
        DTJob *tempJob = [_jobQueue lastObject];
        [_jobQueue removeLastObject];
        if (_nextDwarf != nil && !_nextDwarf.hasJobAssigned) {
            [self assignJob:tempJob toDwarf:_nextDwarf];
        }
    }
}

Forgot to post the last part. This is how the SKScene sends the Job to the Queue. There are two different menus, so I couldn't see how to use the enum for the names here.

#pragma mark - Interaction with the Game
-(void) doSimpleJob:(int)jobNumber forFloor:(int)floorNumber {
    switch (jobNumber) {
        case 0:
            //mining job
            [_game addJobToQueueOfType:MiningJob forFloor:floorNumber];
            break;
        case 1:
            //ladder job
            [_game addJobToQueueOfType:LadderJob forFloor:floorNumber];
            break;
        case 2:
            [self buildBuildingSelectionBoxForFloor:floorNumber];
            _buildingSelectionBoxIsOpen = YES;
            break;
        default:
            break;
    }
}
-(void) doBuildJob:(int)jobNumber{
    switch (jobNumber) {
        case 0:
            //bottom build job
            [_game addJobToQueueOfType:BottomBuildJob forFloor:_selectedFloor];
            break;
        case 1:
            //wall build job
            [_game addJobToQueueOfType:WallBuildJob forFloor:_selectedFloor];
            break;
        case 2:
            //room build job
            [_game addJobToQueueOfType:RoomBuildJob forFloor:_selectedFloor];
            break;
        default:
            break;
    }
}

Answer 1

Let's make your relevant enum more relevant:

typedef NS_ENUM(NSInteger, JobType) {
    MiningJob = 0,
    LadderJob,
    BottomBuildJob,
    WallBuildJob,
    RoomBuildJob
};

Now let's change the argument type of all the methods where we want to send a job type:

As example, get rid of this:

- (void)doBuildJob:(int)jobNumber...

And use this:

- (void)doBuildJob:(JobType)jobType {
    switch (jobType) {
        case BottomBuildJob: 
            // stuff
            break;
        case WallBuildJob: 
            // stuff
            break;
        case RoomBuildJob:
            // stuff
            break;
        case MiningJob:
        case LadderJob:
        default:
            break;
}

There's very little point in typedefing an enum if you're not going to use it as a type. But with that said, typedefing an enum is definitely the right path. We want to make our code more clear, and enums are a great way to do that. typedefing them means we can use them as a type for method arguments.

Also, when switching an a typedef enum, Xcode will help us autocomplete all the types, and warn us if we're missing one. This is why we have the empty cases. Partly to stop Xcode from crying, but mostly to make it explicitly clear that we intend to do nothing in these cases.

This also eliminates the need for comments in some places.

Isn't case MiningJob: much better than case 0: // mining job?

And that's good Objective-C style. Use class, typedef, method, and variable names that help reduce the need for comments. Objective-C is the pinnacle of self-documenting code, so let's be sure we're using its tools to make our code self-documenting.

---

The checkIfFloor:isValidForJob: could be refactored as such:

-(BOOL) checkIfFloor:(int)floorNumber isValidForJob:(int)jobType {
    NSNumber *floorNum = [NSNumber numberWithInt:floorNumber];
    DTTowerFloor *tempFloor = [self.gameTower.towerDict objectForKey:floorNum];
    switch (jobType) {
        case MiningJob:
            return (tempFloor.hasLadder && tempFloor.isRevealed && tempFloor.blockPercentageRemaining > 0);
        case LadderJob:
            return (!tempFloor.hasLadder && tempFloor.isRevealed && self.currentCommonResources >= _ladderCost && !tempFloor.hasActiveWorker);
        case BottomBuildJob:
            return (tempFloor.isRevealed && tempFloor.hasLadder && tempFloor.blockPercentageRemaining <= 0 && !tempFloor.hasBottom && self.currentCommonResources >= _bottomCost && !tempFloor.hasActiveWorker);
        case WallBuildJob:
            return (tempFloor.isRevealed && tempFloor.hasBottom && !tempFloor.hasWalls && self.currentCommonResources >= _wallCost && !tempFloor.hasActiveWorker);
        case RoomBuildJob:
            return (tempFloor.isRevealed && tempFloor.hasBottom && tempFloor.hasWalls && self.currentCommonResources >= _roomCost && !tempFloor.hasActiveWorker);
        default:
            return NO;
    }
}

And that's a good start... but these conditionals are really complicated. What might be best is an individual method for each of these conditions. Where, MiningJob as an example might look like this:

- (BOOL)canDoMiningJob {
    return (self.hasLadder &&
            self.isRevealed &&
            self.blockPercentageRemainig > 0);
}

This would be an instance method on DTTowerFloor.

And then in the above switch statement I mentioned refactoring, we can now just do this...

switch (jobType) {
    case MiningJob:      return [tempFloor canDoMiningJob];
    case LadderJob:      return [tempFloor canDoLadderJob];
    case BottomBuildJob: return [tempFloor canDoBottomBuildJob];
    case WallBuildJob:   return [tempFloor canDoWallBuildJob];
    case RoomBuildJob:   return [tempFloor canDoRoomBuildJob];    
    default:             return NO;
}

---

Now, beyond this, it's hard to make too many comments. You haven't shown us what kind of object _jobQueue is, or how you're using it beyond putting objects into the queue.

For now, I'll assume it's just an NSArray. I'm curious as to why you're inserting new tasks at the beginning of the array. Inserting to the beginning of an array is a particularly expensive operation. Even in Objective-C, where our array simple holds pointers to objects (and pointers are significantly smaller than entire objects and much easier to move around), when we insert at the beginning, every other pointer in the array has to be shifted to the right by 1.

Now, from a gameplay perspective, you can only be handling the queue in one of two ways. It's either first in first out, or first in last out. If it's the former, then tasks are popped out of the queue and completed in the order they were inserted. If it's the latter, then tasks are continually stacked on the queue, and the first task put on the queue isn't completed until every other object put on the queue after it has been completed.

Either way, going with the assumption that your queue is simply an NSArray, I'm going to make the argument that you shouldn't be inserting at the front of the array.

---

Situation One - First In, First Out

In this situation, when tasks are completed, we remove the one that's been in the queue the longest. So, doing it your way and inserting at the front, this is as simple as popping [_jobQueue lastObject] out of the array and performing that task. Modifying the back-end of an array is highly efficient relative to the front end--especially when removing objects.

But, I'm going to make the assumption that players will be able to CANCEL tasks they've put in the queue, and I'm going to make the assumption that modifying the queue is an action that will generally speaking go a lot faster than the game actually completing the tasks. Are these fair assumptions?

If the assumptions are fair, then we want to be adding to the back end of the queue. We can still remove the first object when that task is completed. We can remove any object at any index, just as we can insert any object at any index. And it's not particularly efficient to remove from the front end (it's the same efficiency as inserting at the front end though)... but if player-controlled modification of the queue is likely to happen at a higher frequency (because they can insert/remove all they want) than game-controlled removal from the queue, then we need to put the one that happens less frequently at the less efficient end.

So, insert at the back (simply addOjbect:) and remove from the front when tasks are completed (or from the specific point when a player cancels a tasks).

---

Situation Two - First In, Last Out

I think this might be the less likely scenario, but if this IS the scenario, it makes the decision far easier. If you insert at the front, you also have to remove from the front. If you insert at the back, you remove from the back. Inserting & removing to the front are both highly inefficient (relative to the back) and you're guaranteed to be mostly modifying the front end as this is the end you're inserting to and definitely popping off from. So just use the back end.

---

Ultimately though, the most efficient way to handle the queue though, would be with a doubly-linked list with a head and a tail. But that's a whole different topic. I highly recommend you implement a doubly-linked list to handle the queue, but I feel that is way outside the scope of this QA.