Job Queue for Strategy Game

Question

I've posted a revised version of this here.

I posted about this basic problem a while ago, and I got the recommendation to move the code for managing the Jobs of my game to another class. I have been working on it for a little while now, and I have code that I believe is ready for review. The result is three classes, one for the Queue itself, one for the Job objects that count themselves down, and one for the Units which the Job class creates in order to count itself down. These Units are exposed to Workers when they are in the appropriate state in order to limit the number of workers at one time. The workers do not actually enter the queue, but only enter the Floor that contains the Queue. I'm hoping to hear about any major pitfalls or problems I may be falling into, and also whether or not the design makes sense to other programmers looking at the code.

DTJobQueueStates.h

typedef NS_ENUM(NSInteger, JobQueueStates){
    JobQueueIdle = 0,
    JobQueueWorking,
    JobQueuePaused
};

DTJobQueue.h

#import <Foundation/Foundation.h>
#import "DTJob.h"
#import "DTJobQueueStates.h"

@interface DTJobQueue : NSObject <NSCoding>

-(void) updateJobQueue;

@property JobQueueStates jobQueueState;
@property NSMutableArray *completedJobsForPickup;

-(void) addJob: (DTJob *)job;
-(JobType) activeJobType;
-(void) cancelAllJobs;
-(void) abandonCurrentJob; //used when dwarves die

-(BOOL) areJobSlotsAvailable;
-(int) remainingJobSlots;
-(void) reduceRemainingJobSlots;
-(void) increaseRemainingJobSlots;
-(NSMutableArray *)availableJobSlotsForWork;

-(void) finishOneJobUnit;

-(int) numberOfJobsInArray;

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

@end

DTJobQueue.m

#import "DTJobQueue.h"
#import "DTGroundBlock.h"
#import "DTItem.h"
#import "DTEnemy.h"

@implementation DTJobQueue {
    DTJob *_activeJob;
    NSMutableArray *_jobArray;
}

-(id) init {
    self = [super init];
    if (self) {
        _jobArray = [[NSMutableArray alloc]init];

        _jobQueueState = JobQueueIdle;

        _completedJobsForPickup = [[NSMutableArray alloc]init];

        _blocksOnFloor = [[NSMutableArray alloc]init];
        _itemsOnFloor = [[NSMutableArray alloc]init];
        _enemiesOnFloor = [[NSMutableArray alloc]init];
    }
    return self;
}

#pragma mark - Update Loop
-(void) updateJobQueue {
    switch (self.jobQueueState) {
        case JobQueueIdle:
            if ([self chooseAnActiveJob]) {
                [self startActiveJob];
                self.jobQueueState = JobQueueWorking;
            }
            break;
        case JobQueueWorking:
            [_activeJob updateJob];
            if (_activeJob.status == JobCompleted) {
                [self resolveFinishedJob];
            }
            break;
        default:
            break;
    }
}
-(BOOL) chooseAnActiveJob {
    BOOL choseAJob = NO;
    if (_jobArray.count > 0 && _activeJob == nil) {
        _activeJob = [_jobArray firstObject];
        [_jobArray removeObjectAtIndex:0];
        choseAJob = YES;
    }
    return choseAJob;
}
-(void) startActiveJob {
    switch (_activeJob.jobType) {
        case MiningJob:
            _activeJob.blocksOnFloor = self.blocksOnFloor;
            break;
        case HaulJob:
            _activeJob.itemsOnFloor = self.itemsOnFloor;
            break;
        case FightingJob:
            _activeJob.enemiesOnFloor = self.enemiesOnFloor;
            break;
        default:
            break;
    }
    [_activeJob startJob];
}
-(void) resolveFinishedJob {
    [self.completedJobsForPickup addObject:_activeJob];
    [self clearTemporaryObjects];
    _activeJob = nil;
    self.jobQueueState = JobQueueIdle;
}
-(void) clearTemporaryObjects {
    switch (_activeJob.jobType) {
        case MiningJob:
            [_blocksOnFloor removeAllObjects];
            break;
        case HaulJob:
            [_itemsOnFloor removeAllObjects];
            break;
        case FightingJob:
            [_enemiesOnFloor removeAllObjects];
            break;
        default:
            break;
    }
}

#pragma mark - Job Handling
-(void) addJob: (DTJob *)job {
    if (self.jobQueueState != JobQueuePaused) {
        if (![self alreadyHaveJobOfThisType:job.jobType]) {
            [_jobArray addObject:job];
        }
    }
}
-(BOOL) alreadyHaveJobOfThisType:(JobType)jobType {
    BOOL alreadyHaveJobOfThisType = NO;
    if (_activeJob.jobType == jobType) {
        alreadyHaveJobOfThisType = YES;
    }
    for (DTJob *job in _jobArray) {
        if (job.jobType == jobType) {
            alreadyHaveJobOfThisType = YES;
        }
    }
    return alreadyHaveJobOfThisType;
}
-(BOOL) areJobSlotsAvailable {
    return [_activeJob areJobSlotsAvailable];
}
-(int) remainingJobSlots {
    return _activeJob.remainingJobSlots;
}
-(void) reduceRemainingJobSlots {
    _activeJob.remainingJobSlots--;
}
-(void) increaseRemainingJobSlots {
    _activeJob.remainingJobSlots++;
}
-(void) finishOneJobUnit {
    [_activeJob finishOneJobUnit];
}
-(int) numberOfJobsInArray {
    return (int) _jobArray.count;
}
-(JobType) activeJobType {
    return _activeJob.jobType;
}
-(NSMutableArray *)availableJobSlotsForWork {
    return _activeJob.availableJobSlots;
}

#pragma mark - Cancel Jobs
-(void) cancelAllJobs {
    _activeJob = nil;
    [_jobArray removeAllObjects];
    self.jobQueueState = JobQueueIdle; 
}
-(void) abandonCurrentJob {
    if (_activeJob != nil) {
        [_activeJob abandonCurrentJob];
        [self putActiveJobBackInQueue];
    }
}
-(void) putActiveJobBackInQueue {
    _activeJob.status = JobInQueue;
    [_jobArray addObject:_activeJob];
    _activeJob = nil;
}

#pragma mark - NSCoding methods
//omitted for brevity

@end

DTJobStatus.h

typedef NS_ENUM(NSInteger, JobStatus){
    JobInQueue = 0,
    JobWorkerIsWorking,
    JobCompleted
};

DTJob.h

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

@interface DTJob : NSObject <NSCoding>

-(id) initWithType:(JobType)jobType;

@property JobType jobType;
@property JobStatus status;
@property int floorNumber;
@property int jobUnitsToCreate;
@property int jobUnitsNeededToComplete;
@property NSMutableArray *availableJobSlots;
@property int remainingJobSlots;
@property CGPoint jobPosition;

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

-(BOOL) areJobSlotsAvailable;

-(void) updateJob;
-(void) startJob;
-(void) abandonCurrentJob;
-(void) finishOneJobUnit;

@end

DTJob.m

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

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

static const int kNumJobSlots = 4;

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

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

        _blocksOnFloor = [[NSMutableArray alloc]init];
        _itemsOnFloor = [[NSMutableArray alloc]init];
        _enemiesOnFloor = [[NSMutableArray alloc]init];

        _totalJobSlots = kNumJobSlots;
        _remainingJobSlots = _totalJobSlots;
    }
    return self;
}
-(int) calculateJobsToCreate:(JobType)jobType {
    int numJobsToCreate = 0;
    switch (jobType) {
        case MiningJob:
            numJobsToCreate = 1; //this will be changed to the number of blocks
            break;
        case LadderJob:
            numJobsToCreate = 2;
            break;
        case BottomBuildJob:
            numJobsToCreate = 6;
            break;
        case WallBuildJob:
            numJobsToCreate = 4;
            break;
        case RoomBuildJob:
            numJobsToCreate = 10;
            break;
        case RoomUpgradeJob:
            numJobsToCreate = 1;
            break;
        case SuperiorWallBuildJob:
            numJobsToCreate = 8;
            break;
        case FightingJob:
            numJobsToCreate = 1; //will be changed to number of enemies
            break;
        case CleaningJob:
            numJobsToCreate = 1;
            break;
        case HaulJob:
            numJobsToCreate = 1; //will be changed to number of items
            break;
        default:
            break;
    }
    return numJobsToCreate;
}

#pragma mark - Update loop 
-(void) updateJob {
    [self fillJobSlotsWithJobs];
    [self resolveFinishedJob];
}
-(void) fillJobSlotsWithJobs {
    if (self.availableJobSlots.count < _totalJobSlots) {
        int spotsToFill = _totalJobSlots - (int)self.availableJobSlots.count;
        for (int i = 0; i < spotsToFill ; i++) {
            if (_pendingJobUnits.count > 0) {
                DTJobUnit *jobUnit = [_pendingJobUnits lastObject];
                jobUnit.status = UnitWaitingForWorker;
                [self.availableJobSlots addObject:jobUnit];
                [_pendingJobUnits removeLastObject];
            }
        }
    }
}
-(void) resolveFinishedJob {
    if (_completedJobUnits.count >= self.jobUnitsNeededToComplete) {
        self.status = JobCompleted;
        [self clearAllJobUnits];
    }
}
-(void) clearAllJobUnits {
    [_pendingJobUnits removeAllObjects];
    [_availableJobSlots removeAllObjects];
    [_completedJobUnits removeAllObjects];
}

#pragma mark - Commands
-(void) startJob {
    if (_jobType == MiningJob) {
        //make a job unit for each block on the floor
        for (int i = 0; i < self.blocksOnFloor.count; i++) {
            DTGroundBlock *tempBlock = [self.blocksOnFloor objectAtIndex:i];
            DTJobUnit *jobUnit = [[DTJobUnit alloc]init];
            jobUnit.position = tempBlock.blockPosition;
            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 < self.itemsOnFloor.count; i++) {
            DTItem *tempItem = [self.itemsOnFloor objectAtIndex:i];
            DTJobUnit *jobUnit = [[DTJobUnit alloc]init];
            jobUnit.position = tempItem.position;
            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 < self.enemiesOnFloor.count; i++) {
            DTEnemy *tempEnemy = [self.enemiesOnFloor objectAtIndex:i];
            DTJobUnit *jobUnit = [[DTJobUnit alloc]init];
            jobUnit.position = tempEnemy.enemyMovement.currentPosition;
            jobUnit.status = UnitInQueue;
            [_pendingJobUnits addObject:jobUnit];
        }
        self.jobUnitsNeededToComplete = (int)self.enemiesOnFloor.count;
    } else {
        //make the preset number of jobs
        for (int i = 0; i < self.jobUnitsToCreate; i++) {
            DTJobUnit *jobUnit = [[DTJobUnit alloc]init];
            jobUnit.position = self.jobPosition;
            [_pendingJobUnits addObject:jobUnit];
        }
    }
}
-(BOOL) areJobSlotsAvailable {
    int jobsWaitingForWorkers = 0;
    for (DTJobUnit *jobUnit in self.availableJobSlots) {
        if (jobUnit.status == UnitWaitingForWorker) {
            jobsWaitingForWorkers++;
        }
    }
    if (self.remainingJobSlots > 0 && jobsWaitingForWorkers > 0) {
        return YES;
    } else {
        return NO;
    }
}
-(void) finishOneJobUnit {
    if (self.availableJobSlots.count > 0) {
        DTJobUnit *jobUnit = [self.availableJobSlots firstObject];
        jobUnit.status = UnitCompleted;
        [_completedJobUnits addObject:jobUnit];
        [self.availableJobSlots removeObjectAtIndex:0];
        self.remainingJobSlots++;
    }
}
-(void) abandonCurrentJob {
    int remainingJobsCount = 0;
    for (int i = 0; i < _pendingJobUnits.count; i++) {
        remainingJobsCount++;
    }
    for (int i = 0; i < _availableJobSlots.count; i++) {
        remainingJobsCount++;
    }
    self.jobUnitsToCreate = remainingJobsCount;

    self.remainingJobSlots = _totalJobSlots;
    [self.availableJobSlots removeAllObjects];
    [_pendingJobUnits removeAllObjects];
}

#pragma mark - NSCoding methods
//omitted for brevity

@end

DTJobUnitStatus.h

typedef NS_ENUM(NSInteger, JobUnitStatus){
    UnitInQueue = 0,
    UnitWaitingForWorker,
    UnitWorkerIsWorking,
    UnitCompleted,
};

DTJobUnit.h

#import <Foundation/Foundation.h>
#import "DTJobUnitStatus.h"

@interface DTJobUnit : NSObject <NSCoding>

@property CGPoint position;

@property JobUnitStatus status;

@end

DTJobUnit.m

#import "DTJobUnit.h"

@implementation DTJobUnit

-(id) init {
    self = [super init];
    if (self) {
        _status = UnitInQueue;
    }
    return self;
}

#pragma mark - NSCoding methods
//omitted for brevity

@end

And here is the function to add a new job to the queue. This is in the TowerFloor class:

#pragma mark - Job Handling
-(void) addJob: (DTJob *)job {
    BOOL addJobToArray = NO;
    if (self.floorState == FloorUnderAttack && job.jobType == FightingJob) {
        addJobToArray = YES;
    }
    if (self.floorState == FloorDestroyed && job.jobType == CleaningJob) {
        addJobToArray = YES;
    }
    if (self.floorState != FloorUnderAttack || self.floorState != FloorDestroyed) {
        addJobToArray = YES;
    }
    if (addJobToArray) {
        job.floorNumber = self.floorNumber;
        job.jobPosition = [self setPositionForJob:job.jobType andFloorNumber:job.floorNumber];

        if (job.jobType == MiningJob) {
            for (int i = 0; i < _blocksLeftCount; i++) {
                CGPoint blockPosition = CGPointMake(-_worldSize.width/2.22 + i * _worldSize.width/10, _floorCenterPosition.y);
                [_floorJobQueue.blocksOnFloor addObject:[[DTGroundBlock alloc]initWithPosition:blockPosition]];
            }
        } else if (job.jobType == HaulJob) {
            _floorJobQueue.itemsOnFloor = self.itemArray;
        } else if (job.jobType == FightingJob) {
            _floorJobQueue.enemiesOnFloor = self.enemyArray;
        }
        [_floorJobQueue addJob:job];
    }
}
-(CGPoint) setPositionForJob:(JobType)jobType andFloorNumber:(int)floorNumber {
    CGPoint jobPosition = CGPointZero;
    //special case for building a floor above ground
    if (jobType == BottomBuildJob && floorNumber >= 0) {
        jobPosition = _floorCenterPosition;
    } else {
        //doing this here because the queue does not know the size of the world or position of the floor center
        int randomNumber = arc4random_uniform(4) + 1;
        jobPosition = CGPointMake((_worldSize.width/3)/randomNumber, _floorCenterPosition.y);
    }
    return jobPosition;
}

This is the function that sets the workers working, also in the TowerFloor class:

-(void) assignDwarfMovementForJob:(DTDwarf *)dwarf {
    //assign one job at a time out of the queue
    int jobsToAssign = 1;
    int numJobsAssigned = 0;
    for (DTJobUnit *jobUnit in [_floorJobQueue availableJobSlotsForWork]) {
        if (jobUnit.status == UnitWaitingForWorker && numJobsAssigned < jobsToAssign) {
            dwarf.dwarfMovement.destinationPosition = jobUnit.position;
            dwarf.dwarfMovement.dwarfMovementState = DwarfMovingToJobPosition;
            jobUnit.status = UnitWorkerIsWorking;
            numJobsAssigned++;
            [_floorJobQueue reduceRemainingJobSlots];
        }
    }
}

Finally, this is the method that performs certain actions on the Floor when certain types of job units are completed. This happens when the floor actually registers that the worker is finished working:

-(void) checkDwarvesForWork {
    for (DTDwarf *dwarf in self.dwarfArray) {
        if (dwarf.dwarfMovement.dwarfMovementState == DwarfInPositionForWork && dwarf.dwarfState != DwarfWorking && dwarf.dwarfState != DwarfFinishedWorking) {
            [self startDwarfWorking:dwarf];
        }
        if (dwarf.dwarfState == DwarfFinishedWorking) {
            switch ([_floorJobQueue activeJobType]) {
                case MiningJob:
                    [self doMining];
                    [self moveDwarfToExit:dwarf];
                    break;
                case HaulJob:
                    dwarf.currentItem = [self.itemArray lastObject];
                    [self.itemArray removeLastObject];
                    dwarf.dwarfMovement.dwarfMovementState = DwarfMovingToFloorExit;
                    dwarf.dwarfState = DwarfCarryingItemToStockpile;
                    dwarf.dwarfMovement.destinationPosition = _floorCenterPosition;
                    break;
                case FightingJob:
                {
                    DTEnemy *tempEnemy = [self.enemyArray lastObject];
                    [dwarf calculateFightDamage:tempEnemy.enemyStrength];
                    [self.enemyArray removeLastObject];
                    [self moveDwarfToExit:dwarf];
                    self.hasFloorChanged = YES;
                    if (self.enemyArray.count <= 0) {
                        self.floorState = FloorIdle;
                    }
                    break;
                }
                default:
                    [self moveDwarfToExit:dwarf];
                    break;
            }
            [_floorJobQueue finishOneJobUnit];
        }
    }
}

This all seem pretty complicated, but is it too complicated? Or rather, is it too complicated to understand, or more complicated than it needs to be? I'm open to any criticism you may provide.

Answer 1

Now for a few niggly specific things:

There aren't many obvious things that stick out to me, but I do have a few comments.

typedef NS_ENUM(NSInteger, JobQueueStates){
    JobQueueIdle = 0,
    JobQueueWorking,
    JobQueuePaused
};

This may just be personally preference (though perhaps not, because Apple follows this patterns in their enums, I believe), but I prefer it if every member of my enums is prefixed by the name of the enum itself. So here for example, I'd rename the enum values to the following:

JobQueueStateIdle,
JobQueueStateWorking,
JobQueueStatePaused

The same could be applied to all of the enums.

---

switch (jobType) {
    case MiningJob:
        numJobsToCreate = 1; //this will be changed to the number of blocks
        break;
    case LadderJob:
        numJobsToCreate = 2;
        break;
    case BottomBuildJob:
        numJobsToCreate = 6;
        break;
    case WallBuildJob:
        numJobsToCreate = 4;
        break;
    case RoomBuildJob:
        numJobsToCreate = 10;
        break;
    case RoomUpgradeJob:
        numJobsToCreate = 1;
        break;
    case SuperiorWallBuildJob:
        numJobsToCreate = 8;
        break;
    case FightingJob:
        numJobsToCreate = 1; //will be changed to number of enemies
        break;
    case CleaningJob:
        numJobsToCreate = 1;
        break;
    case HaulJob:
        numJobsToCreate = 1; //will be changed to number of items
        break;
    default:
        break;
}
return numJobsToCreate;

We could instead just do:

switch(jobType) {
    case MiningJob:
        return 1;
    case LadderJob:
        return 2;
// etc all the way down

This save several lines of code and condenses this large switch statement.

Moreover, jobs that are likely to always have the exact same return value can be combined as such:

case RoomUpgradeJob:
case CleaningJob:
    return 1;

---

I think all of your methods returning a BOOL deserve a look:

-(BOOL) alreadyHaveJobOfThisType:(JobType)jobType {
    BOOL alreadyHaveJobOfThisType = NO;
    if (_activeJob.jobType == jobType) {
        alreadyHaveJobOfThisType = YES;
    }
    for (DTJob *job in _jobArray) {
        if (job.jobType == jobType) {
            alreadyHaveJobOfThisType = YES;
        }
    }
    return alreadyHaveJobOfThisType;
}

I'd rewrite this method as such:

-(BOOL) alreadyHaveJobOfThisType:(JobType)jobType {
    if (_activeJob.jobType == jobType) {
        return YES;
    } else {
        for (DTJob *job in _jobArray) {
            if (job.jobType == jobType) {
                return YES;
            }
        }
        return NO;
    }
}

Some people may not like the early returns, but in this case, we're potentially saving a LOT of execution time. Imagine how long it'd take your original version of this method to calculate whether to return YES/NO if you had 50,000 jobs in the array. If there are 50,000 jobs and none of them match what we're checking, both versions take just as long... but if the active job matches, this rewrite doesn't even enter the loop. And if the active job doesn't match, the loop is entered, but the loop is exited as soon as a match is found.

Even if you don't want the early return (people only dislike it because some people think it effects readability, but in small methods like this, it's fine), you should at least let the loop break out early:

for (DTJob *job in _jobArray) {
    if (job.jobType == jobType) {
        alreadyHaveJobOfThisType = YES;
    }
    if (alreadyHaveJobOfThisType) break;
}

---

-(BOOL) areJobSlotsAvailable {
    int jobsWaitingForWorkers = 0;
    for (DTJobUnit *jobUnit in self.availableJobSlots) {
        if (jobUnit.status == UnitWaitingForWorker) {
            jobsWaitingForWorkers++;
        }
    }
    if (self.remainingJobSlots > 0 && jobsWaitingForWorkers > 0) {
        return YES;
    } else {
        return NO;
    }
}

Again, we have a loop that kind of bothers me. We actually don't care how many jobs there are. We only care whether there are some and not none. We also care about self.remainingJobSlots, but we're &&-ing with it, so we can refactor as such:

- (BOOL)areJobSlotsAvailable {
    if (self.remainingJobSlots > 0) {
        for (DTJobUnit *jobUnit in self.availableJobSlots) {
            if (jobUnit.status == UnitWaitingForWorker) {
                return YES;
            }
        }
    }
    return NO;
}

We only check whether or not there is a job waiting for a worker if there are remaining job slots, because if there aren't remaining job slots, the former doesn't matter and we can just return no. Otherwise, we'll check. And when we check, rather than counting, we'll just return yes the first time we encounter one, because again, we don't care about the total count, just whether or not one exists.

Answer 2

This all seem pretty complicated, but is it too complicated? Or rather, is it too complicated to understand, or more complicated than it needs to be?

My answer? I don't know. Maybe. I mean, you're trying to do complicated stuff, so of course the code is going to be complicated. There might be a more simplistic approach, and perhaps someone will come in with a super-answer and give you that more simplistic approach. But here's the problem I have.

This code is complicated. Let's presume that to the best of your knowledge, it's working perfectly as intended right now.

Let's say you spend 6 months doing other things. Maybe you're working on other parts of the game. Or maybe you've released the game and you've started on another project. But 6 months down, a pile of user complaints/comments about the job queue system start to pile up. You want to go straight back to this code base and try and sort it out.

How many hours/days are you going to spend just re-orienting yourself with this code and try to understand it yourself after having not looked at it for 6 months before you understand it to the point that you feel like you can start attempting to make fixes/improvements?

The problem is, you're asking us whether or not it's too complicated, which means you think it's pretty complicated code, but... you've left your future self almost no comments!

There are two "omitted for brevity" comments.

There are three comments left as markers for something that will be changed in the future when more features are added.

None of these five comments are particularly helpful to any future programming trying to fix/improve this code. And outside of these five comments, you've left just 7 comments by my count.

Now, sometimes in Objective-C, it's okay to leave very few comments. We have a tendency to like very long, very verbose method and variable names--we love self-documenting code! But self-documenting code only goes so far. When it comes to trying to understand the bigger picture of what's going on, sometimes we do need some comments.

When I start to write a class that I know will be doing complicated things, the first thing I do at the top of the .h file is a big block quote with a summary of what the class does. A big picture overview in plain English of the class. Additionally, numerous method declarations will have their own block quote with a plain English description of what the intended use of the method is.

Consider if you've hired a programmer to help you. You want him to figure out a bug that has something to do with the job's system. With none of these plain English comments, it can be a bit of a guessing game and he's spending quite a lot of time tracking down where he might need to make some changes.

Answer 3

There's a lot of code here and I probably don't have time to do a proper review.

You are right, it is complicated. It's hard to tell if that is inherent in the task, or the result of your design choices. Almost certainly it's some of each. :-)

Let's look into simplifying some random parts of the code...

assignDwarfMovementForJob:

This is our starting point:

-(void) assignDwarfMovementForJob:(DTDwarf *)dwarf {
    //assign one job at a time out of the queue
    int jobsToAssign = 1;
    int numJobsAssigned = 0;
    for (DTJobUnit *jobUnit in [_floorJobQueue availableJobSlotsForWork]) {
        if (jobUnit.status == UnitWaitingForWorker && numJobsAssigned < jobsToAssign) {
            dwarf.dwarfMovement.destinationPosition = jobUnit.position;
            dwarf.dwarfMovement.dwarfMovementState = DwarfMovingToJobPosition;
            jobUnit.status = UnitWorkerIsWorking;
            numJobsAssigned++;
            [_floorJobQueue reduceRemainingJobSlots];
        }
    }
}

I generally suggest that a method should return to the caller as soon as it discovers it can't do any work, or that it has finished the work. The "one return per method" convention doesn't impress me, especially if it leads to any extra complexity. The meaning of a return statement is utterly clear, in just a couple of seconds, to any non-neophyte developer. On the other hand, some of the extra logic in this method takes much longer to decipher.

Let's try a refactoring to simplify the method a little bit. First, I note that the if statement will always fail once we increment numJobsAssigned. But when the if statement fails, the loop iteration does nothing. Nothing follows the loop, so once we increment numJobsAssigned and finish the body of the if, the method stops doing useful work; we're done. The code should make it much more obvious that we're done. Try this:

-(void) assignDwarfMovementForJob:(DTDwarf *)dwarf {
    //assign one job from the queue if there's an available JobUnit
    for (DTJobUnit *jobUnit in [_floorJobQueue availableJobSlotsForWork]) {
        if (jobUnit.status == UnitWaitingForWorker) {
            dwarf.dwarfMovement.destinationPosition = jobUnit.position;
            dwarf.dwarfMovement.dwarfMovementState = DwarfMovingToJobPosition;
            jobUnit.status = UnitWorkerIsWorking;
            [_floorJobQueue reduceRemainingJobSlots];
            return;
        }
    }
}

So I've eliminated 2 local variables that did nothing but avoid the early return. And I simplified the if condition. I think that's an improvement.

Now the structure of the method is a little clearer to me. I think it does several things:

• It looks for an available JobUnit.
• It does some "stuff" to the Dwarf. I want a good name for the "stuff"; I'll try assignToJobUnit for now. There's probably a better name.
• It does some "stuff" to the available JobUnit. (Changes its status.)
• It does some "stuff" to the JobQueue.

That makes (roughly) four fairly separate activities in one method. But they aren't just separate activities, they happen at different levels of abstraction. I'd rather have a method that does just one or two things, with all the lower-level details (that belong to other levels of abstraction) moved somewhere else.

More worrying, this method knows lots of implementation details about Dwarf, JobQueue, and JobUnit. It knows a Dwarf has a DwarfMovement and a DwarfMovement has a bunch of specific properties. It knows a Queue is built with an Array. And so on. That's too much coupling.

All the decisions in the method are related to the Queue, but the results affect both the Queue and the Dwarf. The Dwarf ends up keeping some information that came from the Queue (jobUnit.position), but the Queue doesn't end up knowing anything about the Dwarf. Consider this scheme:

• Don't let this method rummage around in the other objects. Instead,
• tell the Queue to assign a JobUnit to the Dwarf if it can, with the Queue managing all of its own internal state.
• If the Queue can satisfy the request, it then tells the Dwarf to update its internal state.

Which leads to something like this:

-(void) assignDwarfMovementForJob:(DTDwarf *)dwarf {
    [self.floorJobQueue assignDwarfMovementIfPossible:dwarf];
}

Whoa, that's simple! Now the method only has one major responsibility; it chooses the Queue that will receive the request.

Class JobQueue gets a new method, something like this:

-(void) assignDwarfMovementIfPossible:(DTDwarf *)dwarf {
    for (DTJobUnit *jobUnit in [self availableJobSlotsForWork] {
        if (jobUnit.status == UnitWaitingForWorker) {
            jobUnit.status = UnitWorkerIsWorking;
            [self reduceRemainingJobSlots];

            [dwarf moveToPosition:jobUnit.position];

            return;
        }
    }
}

This method still does two things. It finds an available JobUnit, then it assigns something to it. Maybe it should be broken up even more. But at least it only fiddles with Queue data now. All the Dwarf-fiddling is forwarded to the Dwarf, who takes responsibility for his own internal state.

You could consider calling enumerateObjectsUsingBlock on [self availableJobSlotsForWork] to replace the loop. The if statement and its body would go in the block, which would use the stop parameter instead of return to exit as soon as the work is done.

The Dwarf class gets a method something like this:

-(void) moveToPosition:(CGPoint)position {
    self.dwarfMovement.destinationPositionse = position;
    self.dwarfMovement.dwarfMovementState = DwarfMovingToJobPosition;
}

It looks like we should consider not passing the Dwarf to all these methods, and only pass the DwarfMovement instead. Less coupling is usually better.

After all this refactoring, you might discover you don't need methods like availableJobSlotsForWork that expose implementation details to outside code. You'd want to eliminate such methods when you can.

This is a lot of work. I've gone on for three or four pages, just to explain how I'd refactor one little method. Is it worth the effort? Maybe not, if the code is already complete and working. What I think is worth some effort is learning to recognize, and eliminate, excessive coupling as you write the code. Refactor early and often, before the code gets to the mind-boggling stage.