Revised Job Queue for Strategy Game

Question

After posting my previous question about this Job Queue, I decided I wasn't actually very happy with it. I am embarrassed to admit that upon further testing it did not function properly in all situations. I received some awesome feedback about it, and I have made extensive changes to the classes as well as fixing all of the bugs that I could find. I believe the code is much simpler and easier to understand, so I would like to get some feedback on it.

One of the key differences between this and the previous version is that now the workers are assigned a JobUnit from the JobQueue, and derive their destination positions based on that. Then when they are finished moving, they are sent to the JobQueue to work on that JobUnit. I don't know if this is a good way to handle the problem, but it is working.

DTJobQueue.h:

#import <Foundation/Foundation.h>
#import "DTDwarf.h"
#import "DTJob.h"
#import "DTJobQueueState.h"

@interface DTJobQueue : NSObject <NSCoding>

@property JobQueueState state;
@property NSMutableArray *completedJobsForPickup;

-(void) updateJobQueue;

#pragma mark - Job Handling
-(void) addJob: (DTJob *)job;
-(BOOL) alreadyHaveJobOfThisType:(JobType)jobType;
-(BOOL) areJobsAvailableForWork;
-(JobType) activeJobType;

#pragma mark - Dwarf Handling
-(void) addDwarfToQueue:(DTDwarf *)dwarf;
-(DTJobUnit *)jobUnitForDwarf;

#pragma mark - Pause and Cancel Jobs
-(void) cancelAllJobs;
//nothing currently uses pause
-(void) pauseJobQueue;
-(void) unpauseJobQueue;

#pragma mark - Info for Rendering
-(int) numberOfJobsInArray;
-(NSMutableArray *) listOfJobs;

@end

DTJobQueue.m:

#import "DTJobQueue.h"

/*
    The JobQueue handles the jobs of the floor it is on.
    One job is active at a time, and only one job of each type is allowed at once.
    Each update if a job is not active, the queue tries to make one active and starts it if successful.
    If a job is currently active, it updates it based on workloads received from the workers.
    Once complete it resolves the job and clears workers out of the queue.
    It also has methods to cancel jobs in the queue.
 */

@implementation DTJobQueue {
    DTJob *_activeJob;
    NSMutableArray *_activeJobUnits;
    NSMutableArray *_workerSlots;
    NSMutableArray *_jobQueue;
}

static const int kNumWorkerSlots = 4;

-(id) init {
    self = [super init];
    if (self) {
        _state = JobQueueStateIdle;

        _jobQueue = [[NSMutableArray alloc]init];
        _workerSlots = [[NSMutableArray alloc]init];
        _activeJobUnits = [[NSMutableArray alloc]init];

        _completedJobsForPickup = [[NSMutableArray alloc]init];
    }
    return self;
}

#pragma mark - Update Loop
-(void) updateJobQueue {
    switch (self.state) {
        case JobQueueStateIdle:
            //jobs are started in the idle state and only if one is not already active
            if ([self chooseAnActiveJob]) {
                [self startActiveJob];
                self.state = JobQueueStateWorking;
            }
            break;
        case JobQueueStateWorking:
            //this case happens when a job is active
            //first it loads as many job units as it can, then it checks if the workers have finished any
            //then it resolves the job if all of its units are completed

            [_activeJob updateJob];

            //dont create more new job units than the max minus the number of already loaded units
            int numAvailableJobUnits = kNumWorkerSlots - (int)_activeJobUnits.count;
            //first check if more job units can be loaded
            if ((int)_activeJobUnits.count < kNumWorkerSlots) {
                //if so, load the previously determined number of them, not more than the total number that will be needed
                while ((int)_activeJobUnits.count < _activeJob.jobUnitsNeededToComplete && numAvailableJobUnits > 0) {
                    [self fillJobUnitSlot];
                    numAvailableJobUnits--;
                }
            }

            [self checkForFinishedJobUnits];

            if (_activeJob.status == JobStateCompleted) {
                [self resolveFinishedJob];
            }
            break;
        default:
            break;
    }
}
#pragma mark - Idle Update
-(BOOL) chooseAnActiveJob {
    if (_jobQueue.count > 0 && _activeJob == nil) {
        _activeJob = [_jobQueue firstObject];
        [_jobQueue removeObjectAtIndex:0];
        return YES;
    } else {
        return NO;
    }
}
-(void) startActiveJob {
    //creates the job units
    [_activeJob startJob];
}
#pragma mark - Working Update
-(void) fillJobUnitSlot {
    //returns nil if there are no available job units
    DTJobUnit *jobUnit = [_activeJob jobUnitWaitingForWorker];
    if (jobUnit != nil) {
        jobUnit.status = UnitWaitingForWorker;
        [_activeJobUnits addObject:jobUnit];
    }
}
-(void) checkForFinishedJobUnits {
    //dwarves are removed from their worker slot if their unit is finished
    //they are not passed back and forth, just a reference kept while they are working
    NSMutableArray *dwarvesStaying = [[NSMutableArray alloc]init];
    for (DTDwarf *dwarf in _workerSlots) {
        if (dwarf.dwarfState == DwarfFinishedWorking) {
            dwarf.dwarfState = DwarfReadyForFloorPickup;
            [self completeJobUnit:dwarf.dwarfJobUnit];
        } else {
            [dwarvesStaying addObject:dwarf];
        }
    }
    _workerSlots = dwarvesStaying;
}
-(void) completeJobUnit:(DTJobUnit *)jobUnit {
    [_activeJob completeJobUnit:jobUnit];
    [_activeJobUnits removeObject:jobUnit];
}
-(void) resolveFinishedJob {
    [self.completedJobsForPickup addObject:_activeJob];
    _activeJob = nil;
    self.state = JobQueueStateIdle;
}

#pragma mark - Dwarf Handling
-(void) addDwarfToQueue:(DTDwarf *)dwarf {
    //the dwarf is only added to the queue once in position to work the job
    dwarf.dwarfState = DwarfWorking;
    [_workerSlots addObject:dwarf];
    for (DTJobUnit *jobUnit in _activeJobUnits) {
        //this is a sanity check to make sure the dwarf has the right job unit
        if (jobUnit == dwarf.dwarfJobUnit) {
            jobUnit.status = UnitWorkerIsWorking;
        }
    }
}
-(DTJobUnit *)jobUnitForDwarf {
    //the dwarf is assigned a job unit when it reaches the floor and is assigned a job destination
    DTJobUnit *jobUnit = nil;
    for (int i = 0; i < (int)_activeJobUnits.count; i++) {
        DTJobUnit *tempUnit = [_activeJobUnits objectAtIndex:i];
        if (tempUnit.status == UnitWaitingForWorker) {
            return tempUnit;
        }
    }
    return jobUnit;
}

#pragma mark - Job Handling
-(void) addJob: (DTJob *)job {
    //at this point all other validation has already taken place
    if (self.state != JobQueueStateClosed) {
        [_jobQueue addObject:job];
    }
}
-(BOOL) alreadyHaveJobOfThisType:(JobType)jobType {
    //saving a lot of execution time by returning early
    if (_activeJob.jobType == jobType) {
        return YES;
    } else {
        for (DTJob *job in _jobQueue) {
            if (job.jobType == jobType) {
                return YES;
            }
        }
        return NO;
    }
}
-(BOOL) areJobsAvailableForWork {
    //this method is called by the dwarf movement AI also
    if ([self jobSlotsOpen] && [self jobUnitsAvailableForWork] && self.state == JobQueueStateWorking) {
        return YES;
    } else {
        return NO;
    }
}
-(BOOL) jobSlotsOpen {
    //part of this is a sanity check to prevent too many workers from trying to enter the queue
    if ((int)_workerSlots.count < kNumWorkerSlots && (int)_workerSlots.count < (int)_activeJobUnits.count) {
        return YES;
    } else {
        return NO;
    }
}
-(BOOL) jobUnitsAvailableForWork {
    for (DTJobUnit *jobUnit in _activeJobUnits) {
        if (jobUnit.status == UnitWaitingForWorker || jobUnit.status == UnitWorkerAssigned) {  //temporary workaround
            return YES;
        }
    }
    return NO;
}
-(JobType) activeJobType {
    return _activeJob.jobType;
}

#pragma mark - Pause and Cancel Jobs
-(void) cancelAllJobs {
    [self removeDwarvesFromJobQueue];

    [_activeJobUnits removeAllObjects];
    _activeJob = nil;
    [_jobQueue removeAllObjects];

    self.state = JobQueueStateIdle;
}
-(void) removeDwarvesFromJobQueue {
    for (DTDwarf *dwarf in _workerSlots) {
        dwarf.dwarfState = DwarfStateAbandoningJob;
        dwarf.dwarfJobUnit = nil;
    }
    [_workerSlots removeAllObjects];
}
//currently pausing is not being used by anything
-(void) pauseJobQueue {
    [self removeDwarvesFromJobQueue];

    [self putJobUnitsBackInJob];
    [_activeJobUnits removeAllObjects];

    self.state = JobQueueStatePaused;
}
-(void) unpauseJobQueue {
    if ((int)_jobQueue.count > 0) {
        _activeJob = [_jobQueue firstObject];
        [_jobQueue removeObject:[_jobQueue firstObject]];
        self.state = JobQueueStateWorking;
    } else {
        self.state = JobQueueStateIdle;
    }
}
-(void) putJobUnitsBackInJob {
    for (DTJobUnit *jobUnit in _activeJobUnits) {
        jobUnit.status = UnitInQueue;
    }
}
-(void) putActiveJobBackInQueue {
    if (_activeJob != nil) {
        [_jobQueue insertObject:_activeJob atIndex:0];
        _activeJob = nil;
    }
}

#pragma mark - Info for Rendering
-(NSMutableArray *) listOfJobs {
    NSMutableArray *listOfJobs = [[NSMutableArray alloc]init];

    if (_activeJob != nil) {
        [listOfJobs addObject:_activeJob];
    }
    for (int i = 0; i < (int)_jobQueue.count; i++) {
        [listOfJobs addObject:[_jobQueue objectAtIndex:i]];
    }
    return listOfJobs;
}
-(int) numberOfJobsInArray {
    return (int) _jobQueue.count;
}

#pragma mark - NSCoding methods
-(id) initWithCoder:(NSCoder *)aDecoder {
    self = [super init];
    if (self) {
        _state = [aDecoder decodeIntegerForKey:@"jobQueueState"];
        _completedJobsForPickup = [aDecoder decodeObjectForKey:@"completedJobsForPickup"];
        _activeJob = [aDecoder decodeObjectForKey:@"activeJob"];
        _activeJobUnits = [aDecoder decodeObjectForKey:@"activeJobUnits"];
        _workerSlots = [aDecoder decodeObjectForKey:@"workerSlots"];
        _jobQueue = [aDecoder decodeObjectForKey:@"jobQueue"];
    }
    return self;
}
-(void) encodeWithCoder:(NSCoder *)aCoder {
    [aCoder encodeInteger:self.state forKey:@"jobQueueState"];
    [aCoder encodeObject:self.completedJobsForPickup forKey:@"completedJobsForPickup"];
    [aCoder encodeObject:_activeJob forKey:@"activeJob"];
    [aCoder encodeObject:_activeJobUnits forKey:@"activeJobUnits"];
    [aCoder encodeObject:_workerSlots forKey:@"workerSlots"];
    [aCoder encodeObject:_jobQueue forKey:@"jobQueue"];
}

@end

DTJob.h

#import <Foundation/Foundation.h>
#import "DTJobTypes.h"
#import "DTJobStatus.h"
#import "DTJobUnit.h"

@interface DTJob : NSObject <NSCoding>

-(id) initWithType:(JobType)jobType;

@property JobType jobType;
@property JobState status;

@property CGPoint jobPosition;
@property int floorNumber;
@property int jobUnitsNeededToComplete;

-(void) completeJobUnit:(DTJobUnit *)jobUnit;
-(DTJobUnit *) jobUnitWaitingForWorker;
-(BOOL) jobsAvailable;

-(void) updateJob;
-(void) startJob;
-(void) pauseJob;

@property NSMutableArray *blocksOnFloor;
@property NSMutableArray *itemsOnFloor;
@property NSMutableArray *enemiesOnFloor;

@end

DTJob.m:

#import "DTJob.h"
#import "DTJobUnit.h"
#import "DTGroundBlock.h"
#import "DTItem.h"
#import "DTEnemy.h"

/*
    Jobs are created by the Floors and passed to the JobQueue once validated.
    At this time the Floor gives the job an array of blocks, enemies, or items if the job is Mining, Fighting, or Hauling.
    The JobQueue starts the job, and the Job creates the proper number of JobUnits.
    The JobQueue asks for a valid JobUnit, and then hands it to a dwarf.
    Once completed the JobQueue tells the Job to complete the correct JobUnit.
    Once all JobUnits are completed, the Job changes its state to finished and the JobQueue handles it.
 */

@implementation DTJob {
    int _jobUnitsToCreate;
    NSMutableArray *_pendingJobUnits;
    NSMutableArray *_completedJobUnits;
}

-(id) initWithType:(JobType)jobType {
    self = [super init];
    if (self) {
        _jobType = jobType;
        _jobUnitsToCreate = [self calculateJobsToCreate:jobType];

        _pendingJobUnits = [[NSMutableArray alloc]init];
        _completedJobUnits = [[NSMutableArray alloc]init];

        _blocksOnFloor = [[NSMutableArray alloc]init];
        _itemsOnFloor = [[NSMutableArray alloc]init];
        _enemiesOnFloor = [[NSMutableArray alloc]init];
    }
    return self;
}
-(int) calculateJobsToCreate:(JobType)jobType {
    switch (jobType) {
        case MiningJob:      //this will be changed to the number of blocks
        case RoomUpgradeJob: //different rooms could require more workers but default is 1
        case FightingJob:    //this will be changed to the number of enemies
        case HaulJob:        //this will be changed to the number of items
        case CleaningJob:
            return 1;
        case LadderJob:
            return 2;
        case BottomBuildJob:
            return 6;
        case WallBuildJob:
            return 4;
        case RoomBuildJob:
            return 10;
        case SuperiorWallBuildJob:
            return 8;
        default:
            break;
    }
    return 0;
}

#pragma mark - Update loop 
-(void) updateJob {
    [self checkIfJobIsFinished];
}
-(void) checkIfJobIsFinished {
    if (_completedJobUnits.count >= self.jobUnitsNeededToComplete) {
        self.status = JobStateCompleted;
        [self clearAllJobUnits];
    }
}
-(void) clearAllJobUnits {
    [_pendingJobUnits removeAllObjects];
    [_completedJobUnits removeAllObjects];
}

#pragma mark - Start and Pause Jobs
-(void) startJob {
    if (_jobType == MiningJob) {
        //make a job unit for each block on the floor
        for (int i = 0; i < (int)self.blocksOnFloor.count; i++) {
            DTGroundBlock *tempBlock = [self.blocksOnFloor objectAtIndex:i];
            DTJobUnit *jobUnit = [[DTJobUnit alloc]initWithJobType:_jobType];
            jobUnit.position = tempBlock.blockPosition;
            jobUnit.unitGroundBlock = tempBlock;
            jobUnit.status = UnitInQueue;
            [_pendingJobUnits addObject:jobUnit];
        }
        self.jobUnitsNeededToComplete = (int)self.blocksOnFloor.count;
    } else if (_jobType == HaulJob) {
        //make a job unit for each item on the floor
        for (int i = 0; i < (int)self.itemsOnFloor.count; i++) {
            DTItem *tempItem = [self.itemsOnFloor objectAtIndex:i];
            DTJobUnit *jobUnit = [[DTJobUnit alloc]initWithJobType:_jobType];
            jobUnit.position = tempItem.position;
            jobUnit.unitItem = tempItem;
            jobUnit.status = UnitInQueue;
            [_pendingJobUnits addObject:jobUnit];
        }
        self.jobUnitsNeededToComplete = (int)self.itemsOnFloor.count;
    } else if (_jobType == FightingJob) {
        //make a job unit for each enemy on the floor
        for (int i = 0; i < (int)self.enemiesOnFloor.count; i++) {
            DTEnemy *tempEnemy = [self.enemiesOnFloor objectAtIndex:i];
            DTJobUnit *jobUnit = [[DTJobUnit alloc]initWithJobType:_jobType];
            jobUnit.position = tempEnemy.enemyMovement.currentPosition;
            jobUnit.unitEnemy = tempEnemy;
            jobUnit.status = UnitInQueue;
            [_pendingJobUnits addObject:jobUnit];
        }
        self.jobUnitsNeededToComplete = (int)self.enemiesOnFloor.count;
    } else {
        //if not a special job case, make the preset number of jobs
        for (int i = 0; i < _jobUnitsToCreate; i++) {
            DTJobUnit *jobUnit = [[DTJobUnit alloc]initWithJobType:_jobType];
            jobUnit.position = self.jobPosition;
            jobUnit.status = UnitInQueue;
            //need to make a method that randomly moves this along the x axis for each copy
            [_pendingJobUnits addObject:jobUnit];
        }
        self.jobUnitsNeededToComplete = _jobUnitsToCreate;
    }
}
//pause not currently used by anything
-(void) pauseJob {
    int remainingJobsCount = 0;
    for (int i = 0; i < _pendingJobUnits.count; i++) {
        remainingJobsCount++;
    }
    _jobUnitsToCreate = remainingJobsCount;
    [_pendingJobUnits removeAllObjects];
}

#pragma mark - Job Unit Handling
-(void) completeJobUnit:(DTJobUnit *)jobUnit {
    if (jobUnit != nil) {
        jobUnit.status = UnitCompleted;
        [_completedJobUnits addObject:jobUnit];
        [_pendingJobUnits removeObject:jobUnit];
    }
}
-(DTJobUnit *) jobUnitWaitingForWorker {
    DTJobUnit *jobUnitForWorker = nil;
    for (DTJobUnit *jobUnit in _pendingJobUnits) {
        if (jobUnit.status == UnitInQueue) {
            jobUnitForWorker = jobUnit;
            return jobUnitForWorker;
        }
    }
    return jobUnitForWorker;
}
-(BOOL) jobsAvailable {
    for (DTJobUnit *jobUnit in _pendingJobUnits) {
        if (jobUnit.status == UnitInQueue) {
            return YES;
        }
    }
    return NO;
}

#pragma mark - NSCoding methods
-(id) initWithCoder:(NSCoder *)aDecoder {
    self = [super init];
    if (self) {
        //basic job properties
        _status = [aDecoder decodeIntegerForKey:@"status"];
        _jobType = [aDecoder decodeIntegerForKey:@"jobType"];
        _jobPosition = [aDecoder decodeCGPointForKey:@"jobPosition"];
        _floorNumber = [aDecoder decodeIntForKey:@"floorNumber"];

        //job unit management
        _jobUnitsToCreate = [aDecoder decodeIntForKey:@"jobUnitsToCreate"];
        _jobUnitsNeededToComplete = [aDecoder decodeIntForKey:@"jobUnitsNeededToComplete"];
        _pendingJobUnits = [aDecoder decodeObjectForKey:@"pendingJobUnits"];
        _completedJobUnits = [aDecoder decodeObjectForKey:@"completedJobUnits"];

        //job objects
        _blocksOnFloor = [aDecoder decodeObjectForKey:@"blocksOnFloor"];
        _itemsOnFloor = [aDecoder decodeObjectForKey:@"itemsOnFloor"];
        _enemiesOnFloor = [aDecoder decodeObjectForKey:@"enemiesOnFloor"];
    }
    return self;
}
-(void) encodeWithCoder:(NSCoder *)aCoder {
    //basic job properties
    [aCoder encodeInteger:self.status forKey:@"status"];
    [aCoder encodeInteger:self.jobType forKey:@"jobType"];
    [aCoder encodeInt:self.floorNumber forKey:@"floorNumber"];
    [aCoder encodeCGPoint:self.jobPosition forKey:@"jobPosition"];

    //job unit management
    [aCoder encodeInt:_jobUnitsToCreate forKey:@"jobUnitsToCreate"];
    [aCoder encodeInt:self.jobUnitsNeededToComplete forKey:@"jobUnitsNeededToComplete"];
    [aCoder encodeObject:_pendingJobUnits forKey:@"pendingJobUnits"];
    [aCoder encodeObject:_completedJobUnits forKey:@"completedJobUnits"];

    //job objects
    [aCoder encodeObject:self.blocksOnFloor forKey:@"blocksOnFloor"];
    [aCoder encodeObject:self.itemsOnFloor forKey:@"itemsOnFloor"];
    [aCoder encodeObject:self.enemiesOnFloor forKey:@"enemiesOnFloor"];
}

@end

DTJobUnit.h:

#import <Foundation/Foundation.h>
#import "DTJobUnitStatus.h"
#import "DTJobTypes.h"
#import "DTGroundBlock.h"
#import "DTItem.h"
#import "DTEnemy.h"

@interface DTJobUnit : NSObject <NSCoding>

-(id) initWithJobType:(JobType)jobType;

@property JobUnitStatus status;
@property JobType jobType;

@property CGPoint position;

//these are set by the Job when the JobUnit is created if it is a Mining, Hauling, or Fighting job
@property DTGroundBlock *unitGroundBlock;
@property DTItem *unitItem;
@property DTEnemy *unitEnemy;

@end

DTJobUnit.m:

#import "DTJobUnit.h"

@implementation DTJobUnit

/*
    JobUnits are created by the Job that handles them.
    The Job also sets and checks their status.
    They contain position information to give to dwarves.
    They will contain a reference to a block, item, or enemy if a Mining, Hauling, or Fighting job.
*/

-(id) initWithJobType:(JobType)jobType {
    self = [super init];
    if (self) {
        _status = UnitInQueue;
        _jobType = jobType;
    }
    return self;
}

#pragma mark - NSCoding methods
-(id) initWithCoder:(NSCoder *)aDecoder {
    self = [super init];
    if (self) {
        //basic unit properties
        _status = [aDecoder decodeIntegerForKey:@"status"];
        _position = [aDecoder decodeCGPointForKey:@"position"];
        _jobType = [aDecoder decodeIntegerForKey:@"jobType"];

        //things a unit can reference
        _unitGroundBlock = [aDecoder decodeObjectForKey:@"unitGroundBlock"];
        _unitItem = [aDecoder decodeObjectForKey:@"unitItem"];
        _unitEnemy = [aDecoder decodeObjectForKey:@"unitEnemy"];
    }
    return self;
}
-(void) encodeWithCoder:(NSCoder *)aCoder {
    //basic unit properties
    [aCoder encodeInteger:self.status forKey:@"status"];
    [aCoder encodeCGPoint:self.position forKey:@"position"];
    [aCoder encodeInteger:self.jobType forKey:@"jobType"];

    //things a unit can reference
    [aCoder encodeObject:self.unitGroundBlock forKey:@"unitGroundBlock"];
    [aCoder encodeObject:self.unitItem forKey:@"unitItem"];
    [aCoder encodeObject:self.unitEnemy forKey:@"unitEnemy"];  
}

@end

Here is the method that first assigns a JobUnit to a worker:

-(void) assignDwarfJobUnit:(DTDwarf *)dwarf {
    DTJobUnit *jobUnit = [_floorJobQueue jobUnitForDwarf];
    if (jobUnit != nil) {
        dwarf.dwarfJobUnit = jobUnit;
        dwarf.dwarfMovement.destinationPosition = jobUnit.position;
        dwarf.dwarfMovement.dwarfMovementState = DwarfMovingToJobPosition;
        jobUnit.status = UnitWorkerAssigned;
    }
}

And here is the method that sends the worker to the JobQueue when it is in position:

-(void) putDwarvesToWork {
    for (DTDwarf *dwarf in self.dwarfArray) {
        if (dwarf.dwarfMovement.dwarfMovementState == DwarfAtDestinationFloor || dwarf.dwarfMovement.dwarfMovementState == DwarfAtFloorExit) {
            if (dwarf.dwarfState != DwarfCarryingItemToStockpile) {
                if ([_floorJobQueue areJobsAvailableForWork]) {
                    if ([dwarf isJobAllowed:[_floorJobQueue activeJobType]]) {
                        [self assignDwarfJobUnit:dwarf];
                    } else {
                        dwarf.dwarfMovement.dwarfMovementState = DwarfIdleMovement;
                    }
                } else {
                    dwarf.dwarfMovement.dwarfMovementState = DwarfIdleMovement;
                }
            }
        }
    }
}

I am open to any kind of criticism, so please don't hold back. This is the first time I have tried to extensively comment my code, especially big picture comments that describe what the overall point of a class is, so I would love to know how I did. I did not include the enums this time (this is already a lot of code), and I realized as I posted this that the names still do not perfectly conform to Apple standards, but I will be fixing that next.

Answer 1

A few things about this method bother me:

-(BOOL) chooseAnActiveJob {
    if (_jobQueue.count > 0 && _activeJob == nil) {
        _activeJob = [_jobQueue firstObject];
        [_jobQueue removeObjectAtIndex:0];
        return YES;
    } else {
        return NO;
    }
}

First, An can be completely removed from the method title. It's better as simply: chooseActiveJob. But even this method name is bothersome.

What does this method do? It makes sure there's always an active job, right? There's a WAY better approach.

First, let's change _activeJob to a property:

@property (nonatomic,strong) DTJob *activeJob;

Now, all references to _activeJob will be replaced with self.activeJob, and this method, chooseAnActiveJob can be replaced.

Now, we want to write a custom setter for activeJob.

- (void)setActiveJob:(DTJob *)job {
    if (!job) {
        _activeJob = [_jobQueue firstObject];
    } else {
        _activeJob = job;
    }
    [_jobQueue removeObject:_activeJob];
}

Now, the idea here is that whenever the currently active job is complete, you set:

self.activeJob = nil;

And if you never want to allow the user to do anything other than add jobs to a queue and the queue be executed in order, this is all you have to do. The setter will automatically set the job at the front of the queue to the current active job.

But now, if you want to somehow allow the user to say "Do this job now!", you could send a reference to that job.

The other important thing here is an understanding of how firstObject and removeObject:. These methods (firstObject in particular) are added to prevent having to make clunky index checks.

If the array is empty, firstObject returns nil. And finally, we can call removeObject: to remove whatever object (and any copies of that object) in the job queue, and it's not problematic to send nil to remove either.

As a note, it may be desirably to also start the job that you made the currently active job here.

---

-(void) addJob: (DTJob *)job {
    //at this point all other validation has already taken place
    if (self.state != JobQueueStateClosed) {
        [_jobQueue addObject:job];
    }
}

It would seem to make sense for this method to return a bool based on whether or not the job was added to the queue.

---

-(BOOL) areJobsAvailableForWork {
    //this method is called by the dwarf movement AI also
    if ([self jobSlotsOpen] && [self jobUnitsAvailableForWork] && self.state == JobQueueStateWorking) {
        return YES;
    } else {
        return NO;
    }
}
-(BOOL) jobSlotsOpen {
    //part of this is a sanity check to prevent too many workers from trying to enter the queue
    if ((int)_workerSlots.count < kNumWorkerSlots && (int)_workerSlots.count < (int)_activeJobUnits.count) {
        return YES;
    } else {
        return NO;
    }
}

Any method that follows this form:

if (someCondition) {
    return YES;
} else {
    return NO;
}

Can be rewritten as:

return someCondition;

---

-(NSMutableArray *) listOfJobs {
    NSMutableArray *listOfJobs = [[NSMutableArray alloc]init];

    if (_activeJob != nil) {
        [listOfJobs addObject:_activeJob];
    }
    for (int i = 0; i < (int)_jobQueue.count; i++) {
        [listOfJobs addObject:[_jobQueue objectAtIndex:i]];
    }
    return listOfJobs;
}

First of all, a forin loop will be faster... but what you may not realize is this is horribly inefficient and probably not doing quite what you want. If the order of the jobs is important at all, this method is getting it wrong.

First, you add the _activeJob at the first index (if it exists), then, starting at 0, you copy a job from the job queue into the same index in the list of jobs.

This means first you move the active job from index 0 to 1, then put the job in index 0 of the queue into index 0 of the list. And rinse and repeat for every job in the queue, continuously moving the active job to the very back with each insert.

And while a forin loop is faster than this for loop, what's going to be better is this NSMutableArray method:

[listOfJobs addObjectsFromArray:_jobQueue];

If you want the active job at index 0, then you'll want this:

if (_activeJob) {
    [listOfJobs addObject:_activeJob];
}
[listOfJobs addObjectsFromArray:_jobQueue];

If you want the active job at the back of the array, move the if after the addObjectsFromArray:.

---

_jobUnitsToCreate = [self calculateJobsToCreate:jobType];

This line is in the init for DTJob. Truly, you shouldn't be calling methods within the class for the same reason you shouldn't be using self. syntax to access properties (and instead use the underscore). And with this method, I can make a strong case for it not even being part of the class. I think it still belongs in the file, sure, but you can instead define it as a C-Style function outside the class (before the @interface).

int calculateJobsToCreate(JobType jobType) {
    // copy & paste method body here
}

And now just change init to:

_jobUnitsToCreate = calculateJobsToCreate(jobType);

---

-(void) updateJob {
    [self checkIfJobIsFinished];
}

I don't understand the point of this method really. Truly, the logic from checkIfJobIsFinished could be moved into this one, and eliminate checkIfJobIsFinished (because it's name is clunkier, also, the method name doesn't make a ton of sense given it returns void).

---

Most of the methods in DTJob could have better names. It's the DTJob class, so we know we're dealing with a job, and as such, we don't have to (and shouldn't) include the word "job" in every instance method:

• updateJob could be simply update
• startJob could be simply start
• pauseJob could be simply pause
• clearAllJobUnits could be simply clearAllUnits
• completeJobUnit: could be completeUnit:
• jobUnitWaitingForWorker could be simply unitWaitingForWorker
• jobAvailable could be hasAvailableUnit

---

if (jobUnit != nil)

You do this (or == nil) in a few places. For most Objective-C programmers, simply doing this is preferred:

if (jobUnit) { // jobUnit is not nil

or

if (!jobUnit) { // jobUnit is nil

And you should be doing this also. You're already doing it in all of your init methods:

self = [super init];
if (self) { // self is not nil

So let's be consistent.

---

-(void) putDwarvesToWork {
    for (DTDwarf *dwarf in self.dwarfArray) {
        if (dwarf.dwarfMovement.dwarfMovementState == DwarfAtDestinationFloor || dwarf.dwarfMovement.dwarfMovementState == DwarfAtFloorExit) {
            if (dwarf.dwarfState != DwarfCarryingItemToStockpile) {
                if ([_floorJobQueue areJobsAvailableForWork]) {
                    if ([dwarf isJobAllowed:[_floorJobQueue activeJobType]]) {
                        [self assignDwarfJobUnit:dwarf];
                    } else {
                        dwarf.dwarfMovement.dwarfMovementState = DwarfIdleMovement;
                    }
                } else {
                    dwarf.dwarfMovement.dwarfMovementState = DwarfIdleMovement;
                }
            }
        }
    }
}

This is nested too deeply. The first two levels can be combined in a method:

- (BOOL)canPutDwarfToWork:(DTDwarf *)dwarf {
    if (!(dwarf.dwarfMovement.dwarfMovementState == DwarfAtDestinationFloor ||
        dwarf.dwarfMovement.dwarfMovementState == DwarfAtFloorExit)) {
        return NO;
    }

    if (dwarf.dwarfState == DwarfCarryingItemToStockpile) {
        return NO;
    }

    return YES;
}

We can also combine the inner two most if checks into a single if since the inner else and outer else of these is identical.

Now, we can refactor the loop body:

if ([self canPutDwarfToWork:dwarf]) {
    if ([_floorJobQueue areJobsAvailableForWork] 
        && [dwarf isJobAllowed:[_floorJobQueue activeJobType]]) {
        [self assignDwarfJobUnit:dwarf];
    } else {
        dwarf.dwarfMovement.dwarfMovementState = DwarfIdleMovement;
    }
}

Answer 2

nhgrif gave a very good review and I can’t add much to it.

But I’ll complain about one thing that wasn’t mentioned yet. Your code does some casting, and casting is often an evil that should be avoided as much as possible.

In C, always remember the well-known Golden Rule: "The most common purpose of a cast is to turn a compile-time error into a run-time error." Unless you want run-time errors, try to avoid casts.

Now, objective-C and the Cocoa APIs sometimes force you to cast. You put a DTwarf * into an NSArray, but the NSArray gives you back a crummy id. You know it’s a perfectly good DTDwarf, complete with beard and axe and all the other expected properties. So you cast the id to DTDwarf * -- that's fine, it's not an evil cast, it's an idiom that should really be considered part of the API. [((DTDwarf *)[dwarves objectAtIndex:1]).axe swing] is ugly, but the cast is tolerable here.

I'm objecting to the kinds of casting you might see in an ordinary C program, where too often casts are a symptom of a bad design or a sloppy inplementation. Here are some suggestions:

1. Don't use a cast where C's automatic conversions would do the same thing. If you have an int i and want to put it into a long int li variable, don't say li = (long int) i. Just say li = i and depend on the automatic conversion. If the assignment isn't reasonable, you want the compiler to complain. The cast keeps the compiler from complaining. If someone edits the code so that i is an elephant instead of an int, li = i will probably generate a warning. li = (long int) i probably won't.
2. Declare variables to be the type the API wants. Don't declare them as some other type, and then depend on casts to "fix" the resulting errors.

Example:

    for (int i = 0; i < (int)self.blocksOnFloor.count; i++) {
        DTGroundBlock *tempBlock = [self.blocksOnFloor objectAtIndex:i];
        ...
    }

You don't use this cast consistently in the code, but you use it sometimes. If the compiler doesn't warn about the cast-less versions, you know the cast is unncessary. If it does warn, the cast would just hide a potential bug. The solution is to use a better type.

In this code, i is an int. You compare it to self.blocksOnFloor.count (an NSUInteger), you increment it, and you pass it to objectAtIndex: (which takes an NSUInteger). You should declare i as NSUInteger instead and eliminate the cast:

    for (NSUInteger i = 0; i < self.blocksOnFloor.count; i++) {
        DTGroundBlock *tempBlock = [self.blocksOnFloor objectAtIndex:i];
        ...
    }

Example:

    self.jobUnitsNeededToComplete = (int)self.enemiesOnFloor.count;

What is this cast for? jobUnitsNeededToComplete is declared int. When I try (on Mac OS X):

NSUInteger foo = 10;
int bar = foo;

the compiler says, "Implicit conversion loses integer precision: 'NSUInteger' (aka 'unsigned long') to 'int'". Adding the cast -- int bar = (int)foo; -- eliminates the compiler warning but doesn't fix the real problem. Probably self.jobUnitsNeededToComplete should just be declared NSUInteger instead of int. If some part of your code doesn't agree with that change, there's probably a bug lurking somewhere.