2
\$\begingroup\$

I've been working on a game prototype for a couple days and I think that code review would be really helpful. The basic idea of the game is that you are controlling some dwarves, and you dig up resources underground in order to build a tower above ground.

I'm doing this in Sprite Kit, and initially I wanted to have the dwarves represented by sprites and moved around by Sprite Kit animations. But I thought about it and it seemed to me that if the positions of the dwarves are controlled by the Sprite Kit scene, then it breaks the encapsulation of the game logic, which I am trying to keep totally separate from the scene so that I could possibly port the game to other platforms. So I have the positions of the dwarves controlled by an instance of the Game object which inherits from NSObject. The Sprite Kit scene just renders things based on their positions in the Game.

Is this a correct approach? And also, I am concerned about where I am wasting resources and being inefficient in the program. Please let me know if you need to see any other classes than the following.

First, the Game header:

#import <Foundation/Foundation.h>
#import "DTTower.h"
#import "DTTowerFloor.h"

@interface DTGame : NSObject

@property DTTower *gameTower;
@property NSMutableArray *dwarves;  //this will contain dwarf objects
@property CGSize worldSize;

@property BOOL isPaused;
@property BOOL hasTowerChanged;

//Need the size so that you can use it to position things similarly on iPad and iphone
-(id) initWithSize:(CGSize)size;

-(void) logTower;

-(void) selectFloorForDwarf:(int)floorNumber;

-(void) moveDwarves;
-(void) updateGameCounters;

@end

And now the Game implementation:

#import "DTGame.h"
#import "DTDwarf.h"

@implementation DTGame

//randomness functions
static inline CGFloat skRandf() {
    return rand() / (CGFloat) RAND_MAX;
}
static inline CGFloat skRand(CGFloat low, CGFloat high) {
    return skRandf() * (high - low) + low;
}

#pragma mark - Initialization
-(id) initWithSize:(CGSize)size {
    self = [super init];
    self.worldSize = size;
    [self createDwarves];
    [self createGameTower];
    return self;
}
-(void) createDwarves {
    int numStartingDwarves = 7;
    self.dwarves = [[NSMutableArray alloc]init];
    for (int i = 0; i < numStartingDwarves; i++){
        DTDwarf *tempDwarf = [[DTDwarf alloc]init];
        tempDwarf.currentFloor = 0;
        tempDwarf.worldSize = self.worldSize;
        tempDwarf.position = CGPointMake(0, tempDwarf.currentFloor * self.worldSize.height/6);
        [self.dwarves addObject:tempDwarf];
    }
}
-(void) createGameTower {
    self.gameTower = [[DTTower alloc]init];
}

#pragma mark - Game Update Loop
-(void) updateGameCounters {
    [self countDownWorkTimers];
    [self checkDwarfStatuses];
}
-(void) countDownWorkTimers {
    for (DTDwarf *tempDwarf in self.dwarves) {
        if (tempDwarf.isCountingWork) {
            if ([tempDwarf jobCountdown]) {
                tempDwarf.isCountingWork = NO;
            }
        }
    }
}
-(void) checkDwarfStatuses {
    for (DTDwarf *tempDwarf in self.dwarves) {
        //perform the mining jobs
        if (tempDwarf.hasMiningJobAssigned == YES &&![tempDwarf needsMoving]) {
            [self handleMiningJobs:tempDwarf];
        }

        //set the dwarves to idle if they are not doing anything
        if (tempDwarf.hasMiningJobAssigned == NO && tempDwarf.isMoving == NO && tempDwarf.isCountingWork == NO) {
            tempDwarf.isIdle = YES;
        }
    }
}
-(void) handleMiningJobs:(DTDwarf *)dwarf {
    NSNumber *floorNumber = [NSNumber numberWithInt:dwarf.currentFloor];
    DTTowerFloor *tempFloor = [self.gameTower.towerDict objectForKey:floorNumber];
    if (!dwarf.isCountingWork) {
        dwarf.isCountingWork = YES;
        if (![self.gameTower doMiningToFloor:tempFloor]) {
            dwarf.hasMiningJobAssigned = NO;
            self.hasTowerChanged = YES;
        }
    }
}
//right now this is called from inside the scene time step
-(void) moveDwarves {
    for (DTDwarf *tempDwarf in self.dwarves) {
        if ([tempDwarf needsMoving]) {
            tempDwarf.isMoving = YES;
            tempDwarf.destinationPosition = CGPointMake(0, tempDwarf.destinationFloor * self.worldSize.height/6);
            CGFloat movementDistance = tempDwarf.position.y - tempDwarf.destinationPosition.y;
            if (movementDistance > self.worldSize.height/18 || movementDistance < -self.worldSize.height/18) {
                if (tempDwarf.position.y != tempDwarf.destinationPosition.y){
                    if (movementDistance < 0) {
                        [tempDwarf moveUp];
                    } else {
                        [tempDwarf moveDown];
                    }
                }
            } else {
                tempDwarf.position = tempDwarf.destinationPosition;
                tempDwarf.currentFloor = tempDwarf.destinationFloor;
                tempDwarf.isMoving = NO;
            }
        }
        if (tempDwarf.isIdle) {
            CGPoint tempPosition = CGPointMake(tempDwarf.position.x + skRand(-25, 25),tempDwarf.position.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;
            }
            tempDwarf.position = tempPosition;
        }
    }
}

#pragma mark - Game functions
-(void) selectFloorForDwarf:(int)floorNumber{
    NSNumber *floorNum = [NSNumber numberWithInt:floorNumber];
    DTTowerFloor *tempFloor = [self.gameTower.towerDict objectForKey:floorNum];
    if (tempFloor.hasLadder) {
        [self assignJobToDwarfForFloor:floorNumber];
    }
}
-(void) assignJobToDwarfForFloor:(int)floorNumber {
    for (DTDwarf *tempDwarf in self.dwarves) {
        if (!tempDwarf.hasMiningJobAssigned && !tempDwarf.isMoving) {
            tempDwarf.hasMiningJobAssigned = YES;
            tempDwarf.destinationFloor = floorNumber;
            tempDwarf.isIdle = NO;
            return;
        }
    }
}
-(void) logTower {
    //prints to the console
    [self.gameTower logTower];
}

@end

And now the rendering methods in the SKScene implementation:

#pragma mark - Scene rendering
-(void) renderTower {
    [_sceneTower removeAllChildren];
    for (id key in _game.gameTower.towerDict) {
        DTTowerFloor *tempFloor = [_game.gameTower.towerDict objectForKey:key];
        if (tempFloor.isRevealed && !tempFloor.isBackgroundRevealed) {
            SKSpriteNode *tempFloorSprite = [[SKSpriteNode alloc]init];
            tempFloorSprite.name = @"towerFloor";
            [_sceneTower addChild:tempFloorSprite];
            tempFloorSprite.position = CGPointMake(0, tempFloor.floorNumber * _initialScreenSize.height/6);
            [tempFloorSprite setColor:[SKColor grayColor]];
            tempFloorSprite.size = CGSizeMake(_initialScreenSize.width, _initialScreenSize.height/7);
        } else if (tempFloor.isRevealed && tempFloor.isBackgroundRevealed) {
            SKSpriteNode *tempFloorSprite = [[SKSpriteNode alloc]init];
            tempFloorSprite.name = @"towerBackground";
            [_sceneTower addChild:tempFloorSprite];
            tempFloorSprite.position = CGPointMake(0, tempFloor.floorNumber * _initialScreenSize.height/6);
            [tempFloorSprite setColor:[SKColor greenColor]];
            tempFloorSprite.size = CGSizeMake(_initialScreenSize.width, _initialScreenSize.height/7);
        }
    }
}
-(void) renderTowerFloorBricks {
    [_sceneTowerBricks removeAllChildren];
    for (id key in _game.gameTower.towerDict) {
        DTTowerFloor *tempFloor = [_game.gameTower.towerDict objectForKey:key];
        if (tempFloor.isRevealed) {
            CGFloat blockPercentageRemaining = tempFloor.blockPercentageRemaining;
            for (CGFloat i = 0; i < blockPercentageRemaining; i+=0.10) {
                SKSpriteNode *tempBlock = [[SKSpriteNode alloc]init];
                tempBlock.name = @"towerBlock";
                [tempBlock setColor:[SKColor brownColor]];
                [tempBlock setSize:CGSizeMake(_initialScreenSize.width/10, _initialScreenSize.height/7)];
                tempBlock.position = CGPointMake((-_initialScreenSize.width/2.22 + _initialScreenSize.width*i), tempFloor.floorNumber * _initialScreenSize.height/6);
                [_sceneTowerBricks addChild:tempBlock];
            }
        }
    }
}
-(void) renderTowerOverlay {
    for (id key in _game.gameTower.towerDict) {
        DTTowerFloor *tempFloor = [_game.gameTower.towerDict objectForKey:key];
        SKSpriteNode *tempFloorSprite = [[SKSpriteNode alloc]init];
        tempFloorSprite.name = @"towerOverlay";
        [_sceneTowerOverlay addChild:tempFloorSprite];
        tempFloorSprite.position = CGPointMake(0, tempFloor.floorNumber * _initialScreenSize.height/6);
        [tempFloorSprite setColor:[SKColor colorWithRed:0.00 green:0.00 blue:0.00 alpha:0]];
        tempFloorSprite.size = CGSizeMake(_initialScreenSize.width, _initialScreenSize.height/7);
    }
}
-(void) renderDwarves {
    [_sceneDwarves removeAllChildren];
    for (DTDwarf *tempDwarf in _game.dwarves) {
        SKSpriteNode *tempDwarfSprite = [[SKSpriteNode alloc]init];
        [tempDwarfSprite setColor:[SKColor whiteColor]];
        [tempDwarfSprite setSize:CGSizeMake(_initialScreenSize.width/50, _initialScreenSize.width/50)];
        tempDwarfSprite.position = tempDwarf.position;
        [_sceneDwarves addChild:tempDwarfSprite];
    }
}
\$\endgroup\$
4
\$\begingroup\$

First thing I notice here is the lack of the proper init pattern:

-(id) initWithSize:(CGSize)size {
    self = [super init];
    if (self) {
        self.worldSize = size;
        [self createDwarves];
        [self createGameTower];
    }
    return self;
}

Unfortunately, I'm not exactly certain what could cause self to be nil after [super init] from NSObject, but certainly when you're talking about additional levels of inheritance, returning nil from an init method isn't uncommon. So before we go into doing any sort of initializations, we need do a nil check after the call to super.

Even if we're inheriting from an object we think we know will never return nil, the fact of the matter is, almost every object inherits from NSObject (there are I think two other base classes), and this is the Apple recommended pattern for init methods... which suggests that at least at one time there was a chance for NSObject's init to return nil, and whether or not that chance remains today, the fact that this continues to be the recommended pattern means that could end up happening in the future, and your code would immediately have problems.


The next thing I notice is this:

DTGame.h

#import <Foundation/Foundation.h>
#import "DTTower.h"
#import "DTTowerFloor.h"

The only non-Foundation, non-C type that exists in DTGame.h is DTTower. Moreover, DTTower is a @property, not an argument. So anyone importing DTGame.h certainly doesn't have to know about anything in DTTowerFloor.h to use DTGame objects, and while they MIGHT want to know about DTTower.h in order to set this property, they don't necessarily HAVE to know about it.

So, this isn't actually a run-time issue, and you're not wasting resources, but by importing more headers than you need to in more locations than are necessary, you're slowing down compile times and you're adding a lot of things to auto-complete menus that you don't really need.

DTTowerFloor.h can definitely be moved into the .m. If you need it in another file, you can always import it there too, but there's nothing about DTGame that suggest that anyone using it would also definitely be using DTTowerFloor.

As for DTTower.h, this is a judgment call. You can still use the DTTower property without actually importing the file in the .h by moving the import to the .m, and replacing the .h import with this:

@class DTTower;

This let's the compiler know that the class does definitely exist, and before you use this property, it will be defined, but we don't need the import yet.

But if there's a good chance that most people using DTGame will be using the DTTower property of DTGame, go ahead and leave the import in the .h file.


I don't understand these two public properties:

@property BOOL isPaused;
@property BOOL hasTowerChanged;

The seem odd.

First of all, isPaused straight up isn't used at all in the entire class. And it's fine for a property to not be used. Take the tag property of UIView for example. It's not used at all. It exists simply for someone using UIView to set it to a value so they can retrieve that value later and compare the values. An isPaused seems like a very weird property to do something like this with.

If the game is paused, should any of these actions even take place? If not, and the isPaused property exists on the class, then the class itself should enforce not allowing these actions to take place.

if (self.isPaused) return;

I just don't know. I think isPaused needs a lot of explanation on its intended use, because something is wrong with it.

As for hasTowerChanged, I also don't understand it. I see we're setting it to YES in a certain condition. If the default is NO, we should explicitly set it to NO in our init method.

But more, what's the intended use of this BOOL? Does it ever need to reset to NO?


-(void) selectFloorForDwarf:(int)floorNumber

This should be named to moveDwarfToFloor:(int)floorNumber. It makes it more clear that the argument we're sending is a floor number and that the method takes a dwarf (which the user has no control over which dwarf) and moves him to the specified floor. As named, your method could be confused as some way of automatically selecting a floor for a specific dwarf number to move to.


[self.gameTower.towerDict objectForKey:floorNumber]

You don't include this class here, but this is very suspect. Clearly, towerDict is an NSDictionary property of DTTower. What's unclear to me is why we're using NSNumber objects as keys in the dictionary. Why not just use an array? If we use an array, we don't have to be converting to and from int/NSNumber. We could just stick with int. Or if we want to use a dictionary for DTTower's towerDict property, why is the DTGame class limiting use to NSNumber as the keys via taking int as the argument for moving dwarves to floors? Can't selectFloorForDwarf: (or better moveDwarfToFloor: take an NSObject<NSCopying>* argument so that users can use ANY sort of key they decide on and not be limited to using a dictionary as an array? Or perhaps better yet, just make moveDwarfToFloor: take a DTTowerFloor* argument, and you send a pointer to the floor the dwarf needs to move too, and DTTower can simply maintain an NSSet of all the floor objects.

\$\endgroup\$
  • \$\begingroup\$ Lots of good points here! I am working on some of the suggestions. The reason for isPaused is that the SKScene pauses the game by setting it to isPaused, and the time step only runs while this condition is not true. The reason for hasTowerChanged is that the SKScene only re-renders the tower and some objects when they change, rather than every time step. Thanks for the response! \$\endgroup\$ – bazola Mar 29 '14 at 2:42
  • \$\begingroup\$ I tried to convert everything to an array instead of using an NSDictionary, but when creating the array and trying to set an object to an arbitrary index, it crashes. I start the array at -100 and count up to 4. I guess you can't insert to an arbitrary location in an empty array? It works if I start at index 0 instead of -100, but this messes up the logic in the rest of the program. \$\endgroup\$ – bazola Mar 29 '14 at 16:19
  • 1
    \$\begingroup\$ Arrays cannot have negative indexes. As far as inserting at arbitrary positive indexes, you have to initWithCapacity:, otherwise the array doesn't necessarily have the capacity. It sounds like using a dictionary may end up being easiest, but if this is the case, then we shouldn't limit the floor identifiers to simply ints. In your implementation of the game, it may make sense for simply using ints, but that limits the reusability. \$\endgroup\$ – nhgrif Mar 29 '14 at 18:23

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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