Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

So I've been working on AI for a Tower Building simulation game for quite a few days, and I think the code would really benefit from review. I'm a hobbyist programmer, but I really care about doing things properly so don't be afraid to be harsh. I want to learn.

The basic idea of the job management system is as follows. The user chooses a job with touch commands in the scene. The game evaluates if the job is valid, and if there are enough resources to perform the job. If so, it passes the job to the tower, which passes the job to the appropriate tower floor object.

The dwarves are held in an array by the tower. Each tick, it passes each dwarf an array of floors that contain jobs. The AI of the dwarf chooses which floor to travel to based on its current state, whether all the job slots are full on the floor, and it always moves to the closest floor with available jobs.

When it gets to the floor, the tower passes the dwarf to the floor object. The floor object then tells the dwarf where the position of the job is, and fills one of the active job slots until there are none remaining. When the dwarf reaches the position of the job, the floor sets it to working, and collects the completed job when it is finished. It then tells the dwarf to move to the exit of the floor. The dwarves only move left, right, up and down.

Here is the code.

First the character object, which the dwarves inherit from.

DTCharacter.h:

#import <Foundation/Foundation.h>

@interface DTCharacter : NSObject

//need the world size for proper positioning
@property CGSize worldSize;

@property CGPoint currentPosition;
@property CGPoint destinationPosition;
@property int currentFloor;
@property int destinationFloor;

-(void) moveUp;
-(void) moveDown;
-(void) moveLeft;
-(void) moveRight;

//only used by animals now
@property BOOL isIdle;

@end

DTCharacter.m:

#import "DTCharacter.h"

@implementation DTCharacter

#pragma mark - Movement
-(void) moveUp {
    self.currentPosition = CGPointMake(0, self.currentPosition.y + self.worldSize.height / 18);
}
-(void) moveDown {
    self.currentPosition = CGPointMake(0, self.currentPosition.y - self.worldSize.height / 18);
}
-(void) moveLeft {
    self.currentPosition = CGPointMake(self.currentPosition.x - self.worldSize.width / 18, self.currentPosition.y);
}
-(void) moveRight {
    self.currentPosition = CGPointMake(self.currentPosition.x + self.worldSize.width / 18, self.currentPosition.y);
}

@end

The state enum for dwarves:

typedef NS_ENUM(NSInteger, DwarfStatus){
    Idle = 0,
    NeedsMoving,
    MovingToFloor,
    AtDestinationFloor,
    CarryingItemToStockpile,
    Hungry,
    Eating,
    MovingToJobPosition,
    MovingToFloorExit,
    InPositionForWork,
    Working,
    FinishedWorking,
    AtFloorExit,
    Dead
};

DTDwarf.h:

#import <Foundation/Foundation.h>
#import "DTCharacter.h"
#import "DTDwarfStatus.h"
#import "DTJobTypes.h"

@interface DTDwarf : DTCharacter

//update loop
-(void) checkStatus;

@property DwarfStatus status;

//The tower gives each dwarf this list of floors each tick
@property NSMutableArray *availableFloorsArray;

@property CGPoint floorDestination; //used for horizontal movement

@property JobType jobType;

@end

DTDwarf.m:

#import "DTDwarf.h"
#import "DTJob.h"
#import "DTTowerFloor.h"

@implementation DTDwarf {
    int _miningDelayCount;
    int _currentMiningCount;
    int _ladderBuildDelayCount;
    int _currentLadderCount;
    int _buildDelayCount;
    int _currentBuildCount;
    int _roomUpgradeDelayCount;
    int _currentRoomUpgradeCount;
}

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

    if (self) {
        _status = Idle;
        _jobType = NoCurrentJob;

        //default stats for dwarf
        _miningDelayCount = 8;
        _currentMiningCount = _miningDelayCount;
        _ladderBuildDelayCount = 8;
        _currentLadderCount = _ladderBuildDelayCount;
        _buildDelayCount = 8;
        _currentBuildCount = _buildDelayCount;
        _roomUpgradeDelayCount = 20;
        _currentRoomUpgradeCount = _roomUpgradeDelayCount;

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

#pragma mark - Update Loop
-(void) checkStatus {
    [self pickDestinationFloorForWork];
    switch (self.status) {
        //rest and work states
        case Idle:
            if ([self checkFloorsForJobs]) {
                self.status = NeedsMoving;
            } else {
                [self doIdleMovement];
            }
            break;
        case AtFloorExit:
            self.status = MovingToFloor;
            break;
        case AtDestinationFloor:
            if (![self checkFloorsForJobs]) {
                self.status = MovingToFloorExit;
            }
            break;
        case Working:
            if ([self doJobCountdown]) {
                self.status = FinishedWorking;
            }
            break;

        //Movement states
        case NeedsMoving:
            self.status = MovingToFloorExit;
            break;
        case MovingToFloorExit:
            [self doFloorMovement];
            break;
        case MovingToFloor:
            [self doVerticalMovement];
            break;
        case MovingToJobPosition:
            [self doFloorMovement];
            break;

        default:
            break;
    }
    [self sayStatus];
}
-(void) sayStatus {
    //overriden by subclass
}
-(void) pickDestinationFloorForWork {
    NSMutableArray *floorArray = self.availableFloorsArray;
    NSMutableArray *possibleFloorsArray = [[NSMutableArray alloc]init];
    for (DTTowerFloor *floor in floorArray) {
        if ([floor areJobSlotsAvailable]) {
            [possibleFloorsArray addObject:floor];
        }
    }
    if (possibleFloorsArray.count > 0) {
        self.destinationFloor = [self findClosestFloor:possibleFloorsArray];
    }
}
-(BOOL) checkFloorsForJobs {
    NSMutableArray *floorArray = self.availableFloorsArray;
    for (DTTowerFloor *floor in floorArray) {
        if ([floor areJobSlotsAvailable]) {
            return YES;
        }
    }
    return NO;
}
-(int) findClosestFloor:(NSMutableArray *)possibleFloors {
    int currentFloor = [self calculateCurrentFloorByPosition];
    int destinationFloor = 0;
    int bestCount = 1000; //to guarantee it gets assigned on the first try
    int currentCount = 0;
    for (DTTowerFloor *floor in possibleFloors) {
        //find out what floor has the lowest count of distance
        if (currentFloor > floor.floorNumber) {
            for (int i = currentFloor; i > floor.floorNumber; i--) {
                currentCount++;
            }
        } else {
            for (int i = currentFloor; i < floor.floorNumber; i++) {
                currentCount++;
            }
        }
        //set the destination to the lowest count
        if (currentCount < bestCount) {
            destinationFloor = floor.floorNumber;
            bestCount = currentCount;
            currentCount = 0;
        }
    }
    return destinationFloor;
}
-(int) calculateCurrentFloorByPosition {
    int floorSize = self.worldSize.height/6;
    return self.currentPosition.y / floorSize;
}

#pragma mark - Job Countdowns
-(BOOL) doJobCountdown {
    BOOL didFinishJobCountdown = NO;
        switch (self.jobType) {
            case MiningJob:
                if ([self miningJobCountdown]) {
                    [self resetCountdowns];
                    didFinishJobCountdown = YES;
                }
                break;
            case LadderJob:
                if ([self ladderJobCountdown]) {
                    [self resetCountdowns];
                    didFinishJobCountdown = YES;
                }
                break;
            case BottomBuildJob:
                if ([self buildJobCountdown]) {
                    [self resetCountdowns];
                    didFinishJobCountdown = YES;
                }
                break;
            case WallBuildJob:
                if ([self buildJobCountdown]) {
                    [self resetCountdowns];
                    didFinishJobCountdown = YES;
                }
                break;
            case RoomBuildJob:
                if ([self buildJobCountdown]) {
                    [self resetCountdowns];
                    didFinishJobCountdown = YES;
                }
                break;
            case RoomUpgradeJob:
                if ([self roomUpgradeJobCountdown]) {
                    [self resetCountdowns];
                    didFinishJobCountdown = YES;
                }
                break;
            default:
                break;
        }
    return didFinishJobCountdown;
}
-(BOOL) miningJobCountdown {
    if (_currentMiningCount > 0) {
        _currentMiningCount--;
        return NO;
    } else {
        return YES;
    }
}
-(BOOL) ladderJobCountdown {
    if (_currentLadderCount > 0) {
        _currentLadderCount--;
        return NO;
    } else {
        return YES;
    }
}
-(BOOL) buildJobCountdown {
    if (_currentBuildCount > 0) {
        _currentBuildCount--;
        return NO;
    } else {
        return YES;
    }
}
-(BOOL) roomUpgradeJobCountdown {
    if (_currentRoomUpgradeCount > 0) {
        _currentRoomUpgradeCount--;
        return NO;
    } else {
        return YES;
    }
}
-(void) resetCountdowns {
    _currentMiningCount = _miningDelayCount;
    _currentLadderCount = _ladderBuildDelayCount;
    _currentBuildCount = _buildDelayCount;
    _currentRoomUpgradeCount = _roomUpgradeDelayCount;
}

#pragma mark - Vertical Movement
-(void) doVerticalMovement {
    //the destination floor could change so this needs to update each tick
    [self calculateDestinationPositionByFloor];

    //once close enough to the destination, jump to the destination
    if ([self isAtVerticalDestinationPosition]) {
        [self arriveAtDestinationFloor];
    } else {
        if ([self shouldMoveUp]) {
            [self moveUp];
        } else {
            [self moveDown];
        }
    }
}
-(void) calculateDestinationPositionByFloor {
    int floorSize = self.worldSize.height/6;
    int groundHeightOffset = self.worldSize.height/6/3;
    self.destinationPosition = CGPointMake(0, self.destinationFloor * floorSize - groundHeightOffset);
}
-(BOOL) isAtVerticalDestinationPosition {
    BOOL isAtDestinationPosition = NO;
    int distanceFromDestination = self.currentPosition.y - self.destinationPosition.y;
    int minimumDistance = self.worldSize.height/18;
    if (distanceFromDestination > minimumDistance || distanceFromDestination < -minimumDistance) {
        isAtDestinationPosition = NO;
    } else {
        isAtDestinationPosition = YES;
    }
    return isAtDestinationPosition;
}
-(void) arriveAtDestinationFloor {
    self.currentPosition = self.destinationPosition;
    self.currentFloor = self.destinationFloor;
    self.status = AtDestinationFloor;
}
-(BOOL) shouldMoveUp {
    int distanceFromDestination = self.currentPosition.y - self.destinationPosition.y;
    if (distanceFromDestination < 0) {
        return YES;
    } else {
        return NO;
    }
}

#pragma mark - Horizontal Movement
-(void) doFloorMovement {
    //once close enough to the destination, jump to the destination
    if ([self isAtFloorDestinationPosition]) {
        if (self.status == MovingToFloorExit) {
            self.currentPosition = self.destinationPosition;
            self.status = AtFloorExit;
        }
        if (self.status == MovingToJobPosition) {
            self.currentPosition = self.destinationPosition;
            self.status = InPositionForWork;
        }
    } else {
        if ([self shouldMoveRight]) {
            [self moveRight];
        } else {
            [self moveLeft];
        }
    }
}
-(BOOL) isAtFloorDestinationPosition {
    BOOL isAtDestinationPosition = NO;
    int distanceFromDestination = self.currentPosition.x - self.destinationPosition.x;
    int minimumDistance = self.worldSize.width/18;
    if (distanceFromDestination > minimumDistance || distanceFromDestination < -minimumDistance) {
        isAtDestinationPosition = NO;
    } else {
        isAtDestinationPosition = YES;
    }
    return isAtDestinationPosition;
}
-(BOOL) shouldMoveRight {
    int distanceFromDestination = self.currentPosition.x - self.destinationPosition.x;
    if (distanceFromDestination < 0) {
        return YES;
    } else {
        return NO;
    }
}

#pragma mark - Idle Movement
-(void) doIdleMovement {
    int lowerBoundX = -25;
    int randomHigherBoundX = arc4random_uniform(51);
    int randomPosition = lowerBoundX + randomHigherBoundX;
    CGPoint tempPosition = CGPointMake(self.currentPosition.x + randomPosition, self.currentPosition.y);
    if (tempPosition.x < -self.worldSize.width/2) {
        tempPosition.x = -self.worldSize.width/2 + 50;
    }
    if (tempPosition.x > self.worldSize.width/2){
        tempPosition.x = self.worldSize.width/2 - 50;
    }
    self.currentPosition = tempPosition;
}

@end

DTTowerFloor.h:

#import <Foundation/Foundation.h>
#import "DTGroundTypes.h"
#import "DTRoom.h"
#import "DTJob.h"
#import "DTDwarf.h"

@interface DTTowerFloor : NSObject

-(id) initWithType:(GroundType)groundType andFloor:(int)floorNumber andWorldSize:(CGSize)worldSize;

@property int floorNumber;
@property GroundType groundType;
@property int backgroundType;

//randomly determined at the start
@property int blocksLeftCount;
@property int startingBlockCount;

//update loop
-(void) updateFloor;

//these are built by the workers
//used in the rules to determine if certain buildings are allowed to be built
@property BOOL hasWalls;
@property BOOL hasBottom;
@property BOOL hasLadder;
@property BOOL hasRoom;
@property BOOL hasRoomUpgrade;
@property DTRoom *room;
@property RoomType roomType;

//for rendering
@property BOOL isRevealed;
@property BOOL isBackgroundRevealed;

//these are for the room upgrade jobs
@property int numWorkSpacesLeft;
@property int numWorkers;

//dwarf management
-(void) acceptDwarf:(DTDwarf *)dwarf;
@property NSMutableArray *dwarfArray;
@property NSMutableArray *dwarvesForPickup;

//job management
-(BOOL) areJobSlotsAvailable;
-(void) buildRoomUpgrade:(RoomType)roomType;
-(void) addJob: (DTJob *)job;
@property NSMutableArray *jobArray;
@property NSMutableArray *completedJobsForPickup;

@end

DTTowerFloor.m:

#import "DTTowerFloor.h"

@implementation DTTowerFloor {
    CGPoint _floorCenterPosition;
    CGSize _worldSize;

    DTJob *_activeJob;
    NSMutableArray *_pendingJobUnits;
    NSMutableArray *_availableJobSlots;
    NSMutableArray *_completedJobUnits;

    int _totalJobSlots;
    int _remainingJobSlots;
}

#pragma mark - Initialization
-(id) initWithType:(GroundType)groundType andFloor:(int)floorNumber andWorldSize:(CGSize)worldSize {
    self = [super init];
    if (self) {
        _groundType = groundType;
        _floorNumber = floorNumber;

        [self setBackgroundForGroundType];

        //for routing dwarf movement
        _worldSize = worldSize;
        _floorCenterPosition = CGPointMake(0, _floorNumber * _worldSize.height/6 - _worldSize.height/6/3);

        //number of blocks that will need to be mined out
        _startingBlockCount = [self setInitialBlockCount];
        _blocksLeftCount = _startingBlockCount;

        //number of workers that can work at once
        _totalJobSlots = 4;
        _remainingJobSlots = _totalJobSlots;

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

        _dwarfArray = [[NSMutableArray alloc]init];
        _dwarvesForPickup = [[NSMutableArray alloc]init];
    }
    return self;
}
-(void) setBackgroundForGroundType {
    //eventually this will determine the background based on the type and depth
}
-(int) setInitialBlockCount {
    int randomNumber = arc4random_uniform(7) + 3;
    return randomNumber;
}

#pragma mark - Update loop
-(void) updateFloor {
    //later these would be controlled and activated or disabled when the floor is in different states
    [self chooseAnActiveJob];

    [self startActiveJob];

    [self fillJobSlotsWithJobs];

    [self setPositionsForJobs];

    [self updateFloorDwarves];

    [self putDwarvesToWork];

    [self checkDwarvesForWork];

    [self countFinishedJobs];

    [self putDwarvesInExitQueue];
}
-(void) chooseAnActiveJob {
    if (self.jobArray.count > 0 && _activeJob == nil) {
        _activeJob = [self.jobArray firstObject];
        [self.jobArray removeObjectAtIndex:0];
    }
}
-(void) startActiveJob {
    if (_activeJob != nil && _activeJob.jobsToCreate > 0) {
        int jobCount = 0;
        if (_activeJob.jobType == MiningJob) {
            jobCount = _blocksLeftCount;
            _activeJob.jobUnitsNeeded = jobCount;
        } else {
            jobCount = _activeJob.jobsToCreate;
        }
        for (int i = 0; i < jobCount; i++) {
            //make copies of the job to expand into the pending jobs array
            DTJob *jobCopy = [[DTJob alloc]initWithType:_activeJob.jobType];
            jobCopy.status = InQueue;
            [_pendingJobUnits addObject:jobCopy];
        }
        _activeJob.jobsToCreate = 0;
    }
}
-(void) fillJobSlotsWithJobs {
    if (_availableJobSlots.count < _totalJobSlots) {
        int spotsToFill = _totalJobSlots - _availableJobSlots.count;
        for (int i = 0; i < spotsToFill ; i++) {
            if (_pendingJobUnits.count > 0) {
                [_availableJobSlots addObject:[_pendingJobUnits lastObject]];
                [_pendingJobUnits removeLastObject];
            }
        }
    }
}
-(void) setPositionsForJobs {
    for (DTJob *job in _availableJobSlots) {
        if (job.status == InQueue) {
            // put the job at a semi random x position and at the floor level
            int randomNumber = arc4random_uniform(4) + 1;
            job.jobPosition = CGPointMake((_worldSize.width/3)/randomNumber, _floorCenterPosition.y);
            job.status = WaitingForWorker;
        }
    } 
}
-(void) updateFloorDwarves {
    for (DTDwarf *dwarf in self.dwarfArray) {
        //this is the dwarf update loop
        [dwarf checkStatus];
    }
}
-(void) putDwarvesToWork {
    for (DTDwarf *dwarf in self.dwarfArray) {
        if (dwarf.status == AtDestinationFloor || dwarf.status == AtFloorExit) {
            if ([self areJobSlotsAvailable] && _remainingJobSlots > 0) {
                [self assignDwarfMovementForJob:dwarf];
            } else {
                dwarf.status = Idle;
            }
        }
    }
}
-(void) assignDwarfMovementForJob:(DTDwarf *)dwarf {
    //assign one job at once out of the availableJobSlots array until it is full
    int jobsToAssign = 1;
    int numJobsAssigned = 0;
    for (DTJob *tempJob in _availableJobSlots) {
        if (tempJob.status == WaitingForWorker && numJobsAssigned < jobsToAssign) {
            if (tempJob.jobType == MiningJob) {
                //if its a mining job, place it at the position of the last remaining block out of 10 possible
                int lastBlockNumber = _blocksLeftCount;
                dwarf.destinationPosition = CGPointMake(-_worldSize.width/2.22 + lastBlockNumber * _worldSize.width/10, _floorCenterPosition.y);
            } else {
                //otherwise it was already calculated and set in another method
                dwarf.destinationPosition = tempJob.jobPosition;
            }
            dwarf.status = MovingToJobPosition;
            tempJob.status = WorkerIsWorking;
            numJobsAssigned++;
            _remainingJobSlots--;
        }
    }
}
-(void) checkDwarvesForWork {
    for (DTDwarf *dwarf in self.dwarfArray) {
        if (dwarf.status == InPositionForWork) {
            [self startDwarfWorking:dwarf];
        }
        if (dwarf.status == FinishedWorking) {
            if (_activeJob.jobType == MiningJob) {
                self.blocksLeftCount--;
            } 
            _remainingJobSlots++;
            [self moveDwarfToExit:dwarf];
            [self finishOneJobUnit];
        }
    }
}
-(void) countFinishedJobs {
    if (_activeJob != nil) {
        if (_completedJobUnits.count >= _activeJob.jobUnitsNeeded) {
            _activeJob.status = AllUnitsCompleted;
            [self.completedJobsForPickup addObject:_activeJob];
            _activeJob = nil;
            [self clearAllJobUnits];
        }
    }
}
-(void) clearAllJobUnits {
    [_availableJobSlots removeAllObjects];
    [_completedJobUnits removeAllObjects];
}
-(void) putDwarvesInExitQueue {
    NSMutableArray *dwarvesStaying = [[NSMutableArray alloc]init];
    for (DTDwarf *dwarf in self.dwarfArray) {
        if (dwarf.status == AtFloorExit || dwarf.status == Idle) {
            if (![self areJobSlotsAvailable]) {
                [self.dwarvesForPickup addObject:dwarf];
            } else {
                [dwarvesStaying addObject:dwarf];
            }
        } else {
            [dwarvesStaying addObject:dwarf];
        }
    }
    self.dwarfArray = dwarvesStaying;
}

#pragma mark - Job Handling
-(void) addJob: (DTJob *)job {
    job.status = InQueue;
    job.floorNumber = self.floorNumber;
    [self.jobArray addObject:job];
}
-(BOOL) areJobSlotsAvailable {
    int jobsWaitingForWorkers = 0;
    for (DTJob *job in _availableJobSlots) {
        if (job.status == WaitingForWorker) {
            jobsWaitingForWorkers++;
        }
    }
    if (_remainingJobSlots > 0 && jobsWaitingForWorkers > 0) {
        return YES;
    } else {
        return NO;
    }
}
-(void) finishOneJobUnit {
    if (_availableJobSlots.count > 0) {
        DTJob *job = [_availableJobSlots firstObject];
        job.status = UnitCompleted;
        [_completedJobUnits addObject:job];
        [_availableJobSlots removeObjectAtIndex:0];
    }
}

#pragma mark - Dwarf Handling
-(void) acceptDwarf:(DTDwarf *)dwarf {
    [self.dwarfArray addObject:dwarf];
}
-(void) startDwarfWorking:(DTDwarf *)dwarf {
    dwarf.status = Working;
    dwarf.jobType = _activeJob.jobType;
}
-(void) moveDwarfToExit:(DTDwarf *)dwarf {
    dwarf.status = MovingToFloorExit;
    dwarf.destinationPosition = _floorCenterPosition;
}

#pragma mark - Rooms
//rooms are paid for up front and built instantly, there are many types
-(void) buildRoomUpgrade:(RoomType)roomType {
    self.roomType = roomType;
    self.hasRoomUpgrade = YES;
    self.room = [[DTRoom alloc]initWithRoomType:roomType];
}
share|improve this question

2 Answers 2

up vote 4 down vote accepted

In my answer to one of your previous questions, I believe I pointed you in the direction of NS_ENUM, and it's good to see you using it here (and apologies if you were already using it anyway). Today though, let me introduce you to NS_OPTIONS, which you might want to consider here.

In DTTowerFloor, we have five properties describing different states the room can be in. If these were all mutually exclusive, we'd want to use NS_ENUM, but since they're not, they're a candidate for NS_OPTIONS.

NS_OPTIONS is basically just an NS_ENUM, but it tells the IDE that you intend to bitmask with these variables. This can make certain aspects of your coding more convenient, and you can write some readonly properties with custom accessors to still maintain the convenience of regular BOOL variables for other aspects.

So, here's how we set up NS_OPTIONS, it looks a lot like NS_ENUM:

typedef NS_OPTIONS(NSUInteger, DTTowerFloorState) {
    DTTowerFloorStateNone        = 0, 
    DTTowerFloorStateWalls       = 1 << 0,
    DTTowerFloorStateBottom      = 1 << 1,
    DTTowerFloorStateLadder      = 1 << 2,
    DTTowerFloorStateRoom        = 1 << 3,
    DTTowerFloorStateRoomUpgrade = 1 << 4
};

Now, instead of 5 BOOL variables, our class only needs a single DTTowerFloorState variable.

How this works is we take a block of memory the size of NSUInteger, and each bit in this memory serves as a different flag. The << is shifting the 1 by varying numbers of bits, assigning each one of the values in our options to a different bit place.

The convenience comes in when you're trying to set multiple values at once.

For example, if you need to reset the state of every variable to false, with five BOOL variables, that's five statements setting each to false. With a single bitmask, it's as simple as:

myTowerFloorObject.myTowerFloorState = DTTowerFloorStateNone;

This sets every bit to 0.

Now, as I said, we can still have readonly properties defined to maintain the convenience of a BOOL to check a single value, if that's necessary. That'd look something like this.

Define the property:

@property (nonatomic,readonly,assign) BOOL hasWalls;

Now override the accessor:

- (BOOL)hasWalls {
    return (self.myDTTowerFloorStateVariable & DTTowerFloorStateWalls);
}

This is a single & and is intentional. This is a bitwise and. What this does is return a value with a 1 in every bit that BOTH of the anded variables have a 1 in that position, and a 0 in every other position.

And because in C, false is 0 and true is every other value. DTTowerFloorStateWalls has a 1 in only a single bit position, and your DTTowerFloorState variable will either have a 1 or not in this position. This will return true when your variable does have a 1 in that spot, and false otherwise.

I realize at this point that nothing about NS_OPTIONS sounds easier or better yet.

It is ever so slightly better from a memory standpoint. Realize that a BOOL can only have two values. Yes or no, true or false. But a BOOL variable takes an entire byte of memory. With NS_OPTIONS and bitmasking, every bit represents a bool variable, so you can fit 8 times as many bools in the same memory space. But... we're typically not using so many bits for this to save a significant amount of memory.

So, let me explain some of the convenience of using bitmasking.

There's another bitmask operator we can use when setting this variable, it's the bitwise or, a single pipe: |.

Using this, we can turn multiple states on in a single line of execution:

self.floorState = DTTowerFloorStateWalls | DTTowerFloorStateBottom | DTTowerFloorStateLadder;

The equivalent operation with your current code would look like this:

self.hasWalls = YES;
self.hasBottom = YES;
self.hasLadder = YES;
self.Room = NO;
self.RoomUpgrade = NO;

Notice how the values we don't or in are turned off.

Another convenient operator is the |= operator, which works similar to +=. Let's say we originally set our room as such:

self.floorState = DTTowerFloorStateBottom | DTTowerFloorStateRoom;

Now we want to add walls to the room. If we just do this:

self.floorState = DTTowerFloorStateWalls;

Now we've turned everything else off, but if we instead do:

self.floorState |= DTTowerFloorStateWalls;

Now we've set the walls bit to 1, no matter what it was before, and every other bit remains exactly as it was before adding walls.


(BOOL) checkFloorsForJobs {
    NSMutableArray *floorArray = self.availableFloorsArray;
    for (DTTowerFloor *floor in floorArray) {
        if ([floor areJobSlotsAvailable]) {
            return YES;
        }
    }
    return NO;
}

First of all, if you're creating a new array simple for the sake of fast enumeration, it should be immutable. And really, all of your arrays should be immutable by default. Only make them mutable if you know you're going to mutate them, and here, since we're only fast enumerating, we're definitely not going to mutate the array. But really, there's no reason to create the new variable at all, is there? We can just do this:

for (DTTowerFloor *floor in self.availableFloorsArray) {

We don't need to create another pointer to the same object.


@implementation DTDwarf {
    int _miningDelayCount;
    int _currentMiningCount;
    int _ladderBuildDelayCount;
    int _currentLadderCount;
    int _buildDelayCount;
    int _currentBuildCount;
    int _roomUpgradeDelayCount;
    int _currentRoomUpgradeCount;
}

Are half of these intended to be constants? I can see that all the iVars that begin with _current are actually modified at some point throughout the code. But what about the ones that don't? These look to me like constant default values for your counts.

The same question can be asked of

int _totalJobSlots;
int _remainingJobSlots;

In the Tower class.

Any of these that are constant values and not modified anywhere should be defined as such so that it's clear to the reader that's what they are.

Tomasz's answer already outlines how to define constants. Outside the interface and implementation:

static NSInteger const kMyVariable = 8;

It doesn't have to be an NSInteger either if you don't want. It can be any primitive data type as well as other Objective-C typedefed primitives and NSString * objects.

You should be sure to prefix any constant with a lower case k (and the next letter should be uppercase) as that is the naming tradition for constants. This will help the reader identify and differentiate between constant values and non-constant values.

share|improve this answer
    
Your review is much appreciated. I was not using Enums before one of your previous answers, and I did not know about NS_Options either so that information is very enlightening. I'm going to mark this one as the answer because I think that information would be helpful to others, but both answers were very helpful. Thanks much for your response! –  bazola May 19 at 16:39

I think that DTCharacter object shouldn't know about the world size. You should create some DTWorld class and DTCharacter object should be added to this world. DTCharacter should have and reference to the world and should asking if there is possibility to make a move in specified direction. Maybe good way is to create world as singleton because it's only one. E.g.:

-(void) moveUp {
    CGPoint destination = ...;
    BOOL canMove = [[DTWorld sharedInstance] canMoveToPoint:destination];
    if (canMove) {
        self.currentPosition = destination;
    }
}

Also this is not a good way:

self.currentPosition.y + self.worldSize.height / 18)

Make it like this:

static NSInteger const kStep = 18;
@implementation
...

And in code:

self.currentPosition.y + kStep;

DTDwarf Method -checkStatus should be called update because there is a more things done.

DTTowerFloor Properties as following should be define as (readonly).

@property BOOL hasWalls;
@property BOOL hasBottom;
@property BOOL hasLadder;
@property BOOL hasRoom;
@property BOOL hasRoomUpgrade;

Should be like this:

@property (nonatomic, readonly, getter = hasWalls) BOOL walls;
@property (nonatomic, readonly, getter = hasBottom) BOOL bottom;
...

That's all for now. General rule, try to make your classes with SRP. It's good way to create some class diagram e.g. in Visual Paradigm to check if all looks good. I always start with UML-ing before coding something bigger. Also I think that DTTowerFloor is to clever and some responsibilities should be moved out of this class.

Also you should keep in mind that you should design your classes in the way that this classes expose as little as possible. It simplify class design.

share|improve this answer
    
Your tips are much appreciated. I originally had the Workers be part of the Tower, and have the Tower move them and tell them if they could move. I changed it to this design because I wanted the workers to make their own decision about when and where to move, and move themselves, so they needed to know how much to move (which depends on the world size). About SRP, I am just learning Object Oriented programming so I have not wrapped my head around it fully yet. It would be helpful to hear which pieces of the TowerFloor class should be separated to other classes. Thanks again! –  bazola May 14 at 15:15
1  
I think that Job management and dwarf management should be detached from this class and object like DwarfManager and JobManager should be injected during initialisation process or via properties. You also should think what of properties should be exposed. I think there is too much properties in @interface. You should shift some properties to @implementation and make it "private". Interface of class would be simpler and easier to use. –  Tomasz Szulc May 14 at 18:27
    
Thank you for your answer. I'm finally starting to get the bigger picture of how to set up classes intelligently. –  bazola May 19 at 16:42

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.