• 1

• 25.4k
• 3
• 64
• 129

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.