Ability Cards that trigger Game logic

Question

I want to ask this question because I have been thinking about it for a while and I haven't discovered a solution that I am very happy with.

When coding games, I've frequently run into the situation where it seems advantageous to step out of the OOP mindset and into a sort of functional programming mindset for a brief moment in order to calculate the results of some action. This will usually happen at the Game level, and the results will be applied to any of the objects in the Game as necessary.

In the current situation I have implemented ability cards in my strategy game. The player discovers the cards as they out mine the floors. Once discovered they can be played at any time to gain a one time bonus. This bonus could be anything, so it could need access to the values of the Game such as resources and food, access to the values of the Towers such as the Floors and the Items and Enemies on them, or access to the workers contained in the Tower or in its Floors. Just to be clear, these are things like, "Gain 10 workers," or "Clean up all destroyed floors instantly," or "Pick up all items instantly," or "All workers gain X skill upgrades," or many other similar ideas depending on the balance of the game.

As a result, the logic for what to do is contained in the Game class itself. I don't know if this is bad practice or not. The Card objects simply choose a random type when instantiated, and the types are held in an enum. When a card is played at the Game level, it uses a switch based on the enum values to decide what logic to run.

DTAbilityCardType.h

typedef NS_ENUM(NSInteger, AbilityCardType){
    AbilityCardTypeNone = 0,
    AbilityCardTypeReduceCountdowns,
    AbilityCardTypeDestroyAllEnemies,
    AbilityCardTypePickupAllItems,
    AbilityCardTypeSlowAging,
    AbilityCardTypeGainMigrants
};

DTAbilityCard.h

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

@interface DTAbilityCard : NSObject

@property AbilityCardType type;

@end

DTAbilityCard.m

#import "DTAbilityCard.h"

@implementation DTAbilityCard

//this must be equal to the final number of valid types in the enum
static const int kNumCardTypes = 5;

-(id) init {
    self = [super init];
    if (self) {
        //plus one so none of them are type 0
        int randomNumber = arc4random_uniform(kNumCardTypes) + 1;
        _type = randomNumber;
    }
    return self;
}

@end

Now inside DTGame.m

#pragma mark - Ability Cards
-(void) buildHandOfCards {
    int startingHandSize = 10;
    for (int i = 0; i < startingHandSize; i++) {
        [_abilityCardHand addObject:[[DTAbilityCard alloc]init]];
    }
}
-(void) addNewCardToHand {
    //the type of card will be random
    [_abilityCardHand addObject:[[DTAbilityCard alloc]init]];
}
-(void) playCardNumber:(int)number {
    DTAbilityCard *card = [self.abilityCardHand objectAtIndex:number];
    [self doActionForCardType:card.type];
    [self.abilityCardHand removeObjectAtIndex:number];
}
-(void) doActionForCardType:(AbilityCardType)cardType {
    NSLog(@"played card type = %i", cardType);

    DTTower *activeTower = [_towerArray objectAtIndex:_currentTower];

    switch (cardType) {

        case AbilityCardTypeReduceCountdowns:
            [self reduceCountdownsOnDwarves];
            break;

        case AbilityCardTypeDestroyAllEnemies:
            [self destroyAllEnemies];
            break;

        case AbilityCardTypePickupAllItems:
            [activeTower allItemInstantPickup];
            break;

        case AbilityCardTypeSlowAging:
            [self slowAgingOnDwarves];
            break;

        case AbilityCardTypeGainMigrants:
            [activeTower addMigrantsToTower:10]; //10 is the number of migrants added
            break;

        default:
            break;
    }
}

It seems simple enough and it is pretty self documenting. If I want to add more cards, I add the values to the enum, change the constant in the AbilityCard class to the correct number, add the case to the switch statement in the Game, and then add a method to handle the logic. The worst case scenario, I will have to add one additional method to the Tower class for convenience if it is awkward for the Game to do whatever action is needed.

Ideally for OOP, the actual logic would somehow be encapsulated in each Card object. The only way I can think to do that would be to have the Card object contain a string that was converted to a selector, and use that selector to run a method that will still be in the Game anyway. I can't decide if that is better or just more complicated for no readability gain.

Answer 1

@property AbilityCardType type;

Seems like this should probably be readonly:

@property (nonatomic,readonly) AbilityCardType type;

---

-(void) buildHandOfCards {
    int startingHandSize = 10;
    for (int i = 0; i < startingHandSize; i++) {
        [_abilityCardHand addObject:[[DTAbilityCard alloc]init]];
    }
}
-(void) addNewCardToHand {
    //the type of card will be random
    [_abilityCardHand addObject:[[DTAbilityCard alloc]init]];
}

Given the body of the loop in the first method is just doing exactly what the second method is doing... and the idea that building a hand of cards is nothing more than "drawing" cards until the hand is full, the body of the loop should just call addNewCardToHand each iteration. This way, if we ever want to change an aspect of how we draw cards, we only change it in one place.

startingHandSize should be a constant.

I think a while loop is a little better here. As a programmer, you can perhaps make the assumption that we'll only call buildHandOfCards when we've freshly initialized this object and the hand will start empty. But at the end of the day, what do we really want out of this method? We want a method that makes sure _abilityCardHand has 10 cards in it, right?

- (void)buildHandOfCards {
    // we may want to dump the existing hand if necessary:
    _abilityCardHand = [NSMutableArray array];
    while (_abilityCardHand.count < kStartingHandSize) {
        [_abilityCardHand addObject:[[DTAbilityCard alloc] init];
    }
}

---

[activeTower addMigrantsToTower:10]; //10 is the number of migrants added

This comment is pretty superfluous. The method is called addMigrantsToTower and it takes a number argument. The only way this would need a comment is if the number was something other than the number of migrants to add... in which case the method name might need improvement.

But with this said, again we probably want a constant declared instead of just using 10 here. Put it up there with the starting hand size constant.

---

//plus one so none of them are type 0
int randomNumber = arc4random_uniform(kNumCardTypes) + 1;

If we never want to use AbilityCardTypeNone, then let's not include it?

Moreover, if we don't include an enum value we never want to use at the front end, then as the last value, we can include AbilityCardTypeCount, which, given a 0-based enum, will always be the number we need to use in the random function:

typedef NS_ENUM(NSInteger, AbilityCardType){
    AbilityCardTypeReduceCountdowns = 0,
    AbilityCardTypeDestroyAllEnemies,
    AbilityCardTypePickupAllItems,
    AbilityCardTypeSlowAging,
    AbilityCardTypeGainMigrants,

    AbilityCardTypeCount
};

Then in your init:

_type = arc4random_uniform(AbilityCardTypeCount);

The extra line between GainMigrants and Count helps us remember that any new card types need to be added in this spot. But the point is now we've eliminated one of the steps we need to remember to do when we add a new card type. We don't have to keep up with that kNumCardTypes variable (nor do we need to add 1 to the random result).

---

NSLog(@"played card type = %i", cardType);

We really, really don't want superfluous log statements to make it into the release version of our app. In Xcode, there's a #define for DEBUG which is set to 1 when you build from Xcode into the simulator or device, but when you go to make a release build, Xcode automatically flips it to 0. So, unless we want the log statement to show up in the release version as well, try this:

#if DEBUG = 1
    NSLog(@"played card type = %i", cardType);
#endif

Also, rather than logging the integer value (which isn't necessarily that helpful), why not this:

NSString * abilityCardTypeStringRepresentation(AbilityCardType type) {
    switch(type) {
        case AbilityCardTypeReduceCountdowns: return @"AbilityCardTypeReduceCountdowns";
        case AbilityCardTypeDestroyAllEnemies: return @"AbilityCardTypeDestroyAllEnemies";
        case AbilityCardTypePickupAllItems: return @"AbilityCardTypePickupAllItems";
        case AbilityCardTypeSlowAging: return @"AbilityCardTypeSlowAging";
        case AbilityCardTypeGainMigrants: return @"AbilityCardTypeGainMigrants";
        default: return @"Unknown card type";
    }
}

---

DTTower *activeTower = [_towerArray objectAtIndex:_currentTower];

Let's move this only into the applicable cases, and use modern syntax:

case AbilityCardTypePickupAllItems:
    [_towerArray[_currentTower] allItemInstantPickup];
    break;
case AbilityCardTypeGainMigrants:
    [_towerArray[_currentTower] addMigrantsToTower:kMigrantsToAdd];
    break;

---

-(id) init;

init methods should have a return type of instancetype. I was just reading through some of the iOS 8 API changes, and it looks like Apple has actually made this change in ALL of their classes now (the newer ones were already doing this--the older ones are being changed over).

I'd also like to see this class have a factory method:

+ (instancetype)randomCard {
    return [[self alloc] init];
}

Answer 2

From an OOP perspective, I actually think that Card should not be responsible for carrying out its effects. Card would then have to have references to objects they modify, which increases coupling (the degree to which code is "coupled" together, so to speak). This is actually not beneficial, as it makes your code harder to debug, test, maintain and reuse.

It makes sense that the Game handles this, as it already has all the needed references and so doesn't introduce any overhead in additional memory and etc. In addition, having Card implement it's own logic would mean having a lot of code-sprawl, as each of the cards behavior will have to defined separately, and so make following the logic a bit more difficult (which of course makes debugging more difficult).

Lastly, if you really want to move the card logic out of Game, you can abstract out the card event handling into it's own component, and pass the relevant references there from the main Game class.