Revised Job Queue for Strategy Game

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 
#import "DTDwarf.h"
#import "DTJob.h"
#import "DTJobQueueState.h"

@interface DTJobQueue : NSObject 

@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 cl

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 calculateJo

StackExchange Code Review Q#56477, answer score: 4