Revised worker movement AI for strategy game

A while ago I posted this question about worker AI and Job Queues for a strategy game. I have posted other questions about the Job Queue, and now I would like to post this revised question about specifically the movement class for the workers in the game. I think it would make for a good code review.

The basic idea is that each turn, the update method is triggered which computes the destination floor of a worker based on its current state and the available jobs on different floors. The DwarfMovement class computes the floor to move to, and the Movement class handles the basic movement itself. Enemy, animal, and dwarf movement classes all inherit from the Movement class.

I've spent a lot of time cleaning up this code, but I am sure there are still things that I can improve. I know it is a lot of code, but please feel free to comment on small things. Those types of answers are very helpful too.

DTMovement.h:

#import 

@interface DTMovement : NSObject 

-(id) initWithWorldSize:(CGSize)worldSize;

#pragma mark - Position Properties
@property CGPoint currentPosition;
@property CGPoint destinationPosition;
@property int currentFloor;
@property int destinationFloor;

#pragma mark - Update
-(void) doMovement;

#pragma mark - Advanced Movement
-(void) doIdleMovement;
-(void) doVerticalMovement;
-(void) doFloorMovement;
-(void) pickRandomDestinationOnCurrentFloor;

#pragma mark - Simple Movement
-(void) moveUp;
-(void) moveDown;
-(void) moveLeft;
-(void) moveRight;

#pragma mark - Postion Calculations
-(int) currentFloorByPosition;
-(int) closestFloor:(NSMutableArray *)possibleFloors;

-(void) calculateFloorExitPositionByFloor;
-(void) calculateDestinationPositionByFloor;

-(BOOL) isAtVerticalDestinationPosition;
-(BOOL) isAtFloorDestinationPosition;
-(BOOL) shouldMoveUp;
-(BOOL) shouldMoveRight;

#pragma mark - Tower Change
-(void) transitionToNewTower;

@end

DTMovement.m:

```
#import "DTMovement.h"
#import "DTTowerFloor.h"

static int const kMinVertical

-(void) tryToChooseAJobWhileIdle

You don't need to say "try". I think this method name is fine as "chooseJobWhileIdle" or maybe even "lookForJob", "findJob", or maybe "idleJobSearch" perhaps. The current method name is verbose, yes, and we like that in Objective-C. We want to be descriptive and self-documenting, but not overly-so.

The method name should convey to the developer the important thing that the method does. It's not particularly important to the developer that this method may fail to find a job. And if it were, how would the developer know the dwarf failed to find a job? If we want to let the developer know that this method might fail to find a job, we can let him know by returning a BOOL indicating success/failure, and letting him potentially act on that knowledge.

-(void) pickDestinationFloorForEating

You have a handful of methods like this, and the logic is the same in all of them. The logic should be combined into a single method. I'm fining keeping these methods here as convenience methods, but the actual logic should exist in a single method. This may mean refactoring the actual logic some, but in the end, it will be better when you go back to adding more of these sorts of methods.

if (currentFloor > floor.floorNumber) {
    for (int i = currentFloor; i > floor.floorNumber; i--) {
        currentCount++;
    }
} else {
    for (int i = currentFloor; i < floor.floorNumber; i++) {
        currentCount++;
    }
}

Not that loops within loops can always be avoided, but they should be a bit of a flag to stop and look to make sure they can't be avoided. A loop is an effective way to get a small block of code to execute several times... so a loop in a loop is a way of getting a loop (which will execute its body several times each time) to execute several times. You can spend a lot of time executing nested loops.

Here, it's unnecessary. All of that code is replaced with this:

currentCount += abs(currentFloor - floor.floorNumber);

You remove the time it took to check the if conditional each run through the outer loop, the time it took to check the for conditional, and increment/decrement two variables each time through the inner loop. You also completely remove the loop iteration variable.

And honestly, in that method I think this is the only place currentCount is modified, so we can get rid of resetting it to 0 at the end of each loop, and simple do this:

currentCount = abs(currentFloor - floor.floorNumber);

Which is probably slightly more efficient than the += and certainly saves the time resetting currentCount to 0 on each iteration.

StackExchange Code Review Q#57645, answer score: 4