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 am sure there are better ways to set this up, but I wanted to see how something like this would work in Objective-C, so I went ahead and built a prototype of a Tradeable Card Game. I've never loaded information from a plist before, so this was the perfect opportunity for me to try that.

The idea here is that the plist will contain all the relevant statistical information for all of the cards in the game in a bunch of dictionaries, each one of them representing a card. Each of those dictionaries may or may not have certain properties attached to predetermined keys. Note, it may not have the key and value at all if that specific card does not need those properties.

When the Game is created, it builds a CardList object using this plist which simply holds an array of these card dictionaries in the order that it finds them in the plist. Then when asked, the CardList object can spit out a Card object that is either random, or determined by its cardId, which would also be its index in the CardList array. I don't know if this is a completely sane way to set this up, but it is working.

One concern is that once the game was "released", the order inside the plist could not be changed easily. However this might be mitigated by using a CardCollection object to hold the permanent storage of cards for a given player from which they can construct their decks. Once a Card is created out of the CardList, only its properties matter and not how it got them. The Game is only building the decks manually in this way for initial testing.

I'll put the plist first so you can see what kind of data the Card objects need to have.

CardListPlist.plist

<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
    <dict>
        <key>Cards</key>
        <array>
            <dict>

                <key>cardId</key>
                <string>0001</string>
                <key>cardName</key>
                <string>TestCard01</string>
                <key>cardType</key>
                <string>0</string>
                <key>unitStrength</key>
                <string>3</string>
                <key>unitHealth</key>
                <string>4</string>
                <key>abilitySlots</key>
                <string>2</string>

            </dict>
            <dict>

                <key>cardId</key>
                <string>0002</string>
                <key>cardName</key>
                <string>TestCard02</string>
                <key>cardType</key>
                <string>0</string>
                <key>unitStrength</key>
                <string>6</string>
                <key>unitHealth</key>
                <string>6</string>
                <key>abilitySlots</key>
                <string>0</string>

            </dict>
            <dict>

                <key>cardId</key>
                <string>0003</string>
                <key>cardName</key>
                <string>TestCard03</string>
                <key>cardType</key>
                <string>1</string>
                <key>abilitySlots</key>
                <string>2</string>

            </dict>
        </array>
    </dict>
</plist>

CardList.h

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

@interface CRGCardList : NSObject

@property (strong, nonatomic) NSArray *listOfAllCards;

-(void) buildCardList;
-(void) logCards;

-(CRGCard *) randomCardFromCardList;
-(CRGCard *) cardWithId:(int)cardId;

@end

CardList.m

#import "CRGCardList.h"

NSString* const kCardStrength = @"unitStrength";
NSString* const kCardHealth = @"unitHealth";
NSString* const kCardType = @"cardType";
NSString* const kCardAbilitySlots = @"abilitySlots";
NSString* const kCardName = @"cardName";
NSString* const kCardId = @"cardId";

@implementation CRGCardList

#pragma mark - Initialization
-(id) init {
    self = [super init];
    if (self) {
        _listOfAllCards = [[NSArray alloc]init];
    }
    return self;
}
-(void) buildCardList {
    NSString *plistCatPath = [[NSBundle mainBundle] pathForResource:@"CRGCardListPlist" ofType:@"plist"];
    NSDictionary *cardDictionary = [[NSDictionary alloc] initWithContentsOfFile:plistCatPath];
    self.listOfAllCards  = cardDictionary[@"Cards"];
}

#pragma mark - Card building
-(CRGCard *) randomCardFromCardList {
    int randomNumber = arc4random_uniform(_listOfAllCards.count);
    NSDictionary *cardDict = [_listOfAllCards objectAtIndex:randomNumber];
    CRGCard *card = [[CRGCard alloc]initWithId:[self convertStringToInt:[cardDict objectForKey:kCardId]]
                                          name:[cardDict objectForKey:kCardName]
                                          type:[self convertStringToInt:[cardDict objectForKey:kCardType]]
                                  unitStrength:[self convertStringToInt:[cardDict objectForKey:kCardStrength]]
                                    unitHealth:[self convertStringToInt:[cardDict objectForKey:kCardHealth]]
                                  abilitySlots:[self convertStringToInt:[cardDict objectForKey:kCardAbilitySlots]]];
    return card;
}
-(CRGCard *) cardWithId:(int)cardId {
    NSDictionary *cardDict = [_listOfAllCards objectAtIndex:cardId];
    CRGCard *card = [[CRGCard alloc]initWithId:[self convertStringToInt:[cardDict objectForKey:kCardId]]
                                          name:[cardDict objectForKey:kCardName]
                                          type:[self convertStringToInt:[cardDict objectForKey:kCardType]]];
    return card;
}
-(int) convertStringToInt:(NSString *)string {
    return [string intValue];
}

#pragma mark - Diagnostic Methods
-(void) logCards {
    for (NSDictionary *dict in self.listOfAllCards) {
        for (id key in dict) {
            NSString *string = [dict objectForKey:key];
            NSLog(@"%@", string);
        }
    }
}

@end

Card.h

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

@interface CRGCard : NSObject

//not all cards will have all the values, so there could be different factory methods, here there is one for units and a default one

-(instancetype) initWithId:(int)cardId name:(NSString *)name type:(CardType)type unitStrength:(int)unitStrength unitHealth:(int)unitHealth abilitySlots:(int)abilitySlots;
-(instancetype) initWithId:(int)cardId name:(NSString *)name type:(CardType)type;

@property int cardId;
@property NSString* name;
@property CardType type;

@property int unitStrength;
@property int unitHealth;
@property int abilitySlots;

@end

Card.m

#import "CRGCard.h"

@implementation CRGCard

-(instancetype) initWithId:(int)cardId name:(NSString *)name type:(CardType)type unitStrength:(int)unitStrength unitHealth:(int)unitHealth abilitySlots:(int)abilitySlots {
    self = [super init];
    if (self) {
        _cardId = cardId;
        _name = name;
        _type = type;
        _unitStrength = unitStrength;
        _unitHealth = unitHealth;
        _abilitySlots = abilitySlots;
    }
    return self;
}

-(instancetype) initWithId:(int)cardId name:(NSString *)name type:(CardType)type {
    self = [super init];
    if (self) {
        _cardId = cardId;
        _name = name;
        _type = type;
    }
    return self;
}

@end

Game.h

#import <Foundation/Foundation.h>

@interface CRGGame : NSObject

-(void) startGame;

@property NSMutableArray *players;

@end

Game.m

#import "CRGGame.h"
#import "CRGCardList.h"
#import "CRGPlayer.h"

static const int kDeckSize = 30;

@implementation CRGGame {
    CRGCardList *_allCardsList;
}

-(id) init {
    self = [super init];
    if (self) {
        _players = [[NSMutableArray alloc]init];
        _allCardsList = [[CRGCardList alloc]init];
    }
    return self;
}

-(void) startGame {
    [_allCardsList buildCardList];
    [_allCardsList logCards];
    [self createPlayers];
    [self createDecks];
    [self drawStaringHands];
}
-(void) createPlayers {
    int numPlayers = 2;
    for (int i = 0; i < numPlayers; i++) {
        [_players addObject:[[CRGPlayer alloc]init]];
    }
}

#pragma mark - Build Decks
-(void) createDecks {
    for (CRGPlayer *player in self.players) {
        [self fillDeckWithRandomCards:player.deck];
    }
}
-(void) fillDeckWithRandomCards:(NSMutableArray *)deck {
    for (int i = 0; i < kDeckSize; i++) {
        [deck addObject:[_allCardsList randomCardFromCardList]];
    }
}

#pragma mark - Draw Hands
-(void) drawStaringHands {
    for (CRGPlayer *player in self.players) {
        [player drawStartingHand];
    }
}

@end

Player.h

#import <Foundation/Foundation.h>

@interface CRGPlayer : NSObject

@property NSMutableArray *deck;
@property NSMutableArray *hand;

-(void) drawStartingHand;

@end

Player.m

#import "CRGPlayer.h"

static const int kStartingHandSize = 3;

@implementation CRGPlayer

-(instancetype) init {
    self = [super init];
    if (self) {
        _deck = [[NSMutableArray alloc]init];
        _hand = [[NSMutableArray alloc]init];
    }
    return self;
}

-(void) drawStartingHand {
    for (int i = 0; i < kStartingHandSize; i++) {
        [self drawCardFromDeck];
    }
}

-(void) drawCardFromDeck {
    [_hand addObject:[_deck lastObject]];
    [_deck removeLastObject];
}

@end

I realize this is a lot of code, but I felt like it was necessary to see all of it together in order to really understand what I am trying to do. What I am most interested in hearing about is the way that I have the plists set up, and whether this is a good approach or not. Also, is the approach properly OOP?

share|improve this question

3 Answers 3

up vote 2 down vote accepted

Game.h/Game.m

This is a really big question, so I'm going to address one thing at a time...


@interface CRGGame : NSObject

-(void) startGame;

@property NSMutableArray *players;

@end

We have to consider right now whether we want the number of players in our game to be something we can change in the middle of our game. If it's not something we should be changing, then perhaps change to an interface more like this;

@interface CRGGame : NSObject

- (void)startGameWithPlayers:(NSArray *)players;

@property (readonly) NSArray *players;

@end

What this means is that the object wanting to run a game is responsible for creating the players that should be participating in the game. This actually probably makes a lot more sense from an OOP standpoint, and from a usability standpoint.

Consider in a practical scenario, we push a "Start New Game" button, and the first thing that happens is we set up the people playing. We stay in this player set up area until we're done setting up all the players. Some view controller is responsible for creating and setting up the players, shoving them into an array, and then starting the game while passing the array of players to us.

We may also want to add methods for pausing the game, saving the game (to permanent storage so it can be resumed later), and ending the game.


Although honestly, it seems a good idea to setup Game to use delegation. Create a GameDataSource and a GameDelegate protocol.

Calling startGame (without the array) would cause Game to ask the data source for the array of players... or probably better yet, the datasource is asked for the number of players, then it asks for the players one at a time. This can ensure we're getting player objects instead of an array that may or may not include non-player objects.


-(void) startGame {
    [_allCardsList buildCardList];
    [_allCardsList logCards];
    [self createPlayers];
    [self createDecks];
    [self drawStaringHands];
}

I know we already fixed this in CardList.m, but we should still also wrap debugging stuff in #if DEBUG statements:

#if DEBUG
    [_allCardsList logCards];
#endif

We can also completely eliminate createPlayers since we're relying on the object using us to pass us an array of players now.


-(void) fillDeckWithRandomCards:(NSMutableArray *)deck {
    for (int i = 0; i < kDeckSize; i++) {
        [deck addObject:[_allCardsList randomCardFromCardList]];
    }
}

As this is a fill method, to me it says "We're going to draw until there are this many cards in the deck", in which case, we should probably use a while loop with a condition like this:

while (deck.count < kDeckSize)

Meanwhile, we might provide another method for adding an explicit number of cards to the deck:

- (void)addRandomCards:(int)cardsToAdd toDeck:(NSMutableArray *)deck;

And this method would use a for loop just like your existing method, except the conditional part would be: i < cardsToAdd


-(void) drawStaringHands {
    for (CRGPlayer *player in self.players) {
        [player drawStartingHand];
    }
}

There's a small typo here. StaringHands seems a little creepy...

share|improve this answer

CardList.h/CardList.m

This is a really big question, so I'm going to address one thing at a time...


One concern is that once the game was "released", the order inside the plist could not be changed easily.

So, instead of embedding the plist in the app, we instead ask the app to connect to the Internet and download the card properties from our server. Given that any update to an iOS app takes 3-7 days, it's good to be able to move portions of our app out of the source code and into remote files. Where we can update our app without going through Apple's review process, we should.


-(CRGCard *) randomCardFromCardList {
    int randomNumber = arc4random_uniform(_listOfAllCards.count);
    NSDictionary *cardDict = [_listOfAllCards objectAtIndex:randomNumber];
    CRGCard *card = [[CRGCard alloc]initWithId:[self convertStringToInt:[cardDict objectForKey:kCardId]]
                                          name:[cardDict objectForKey:kCardName]
                                          type:[self convertStringToInt:[cardDict objectForKey:kCardType]]
                                  unitStrength:[self convertStringToInt:[cardDict objectForKey:kCardStrength]]
                                    unitHealth:[self convertStringToInt:[cardDict objectForKey:kCardHealth]]
                                  abilitySlots:[self convertStringToInt:[cardDict objectForKey:kCardAbilitySlots]]];
    return card;
}
-(CRGCard *) cardWithId:(int)cardId {
    NSDictionary *cardDict = [_listOfAllCards objectAtIndex:cardId];
    CRGCard *card = [[CRGCard alloc]initWithId:[self convertStringToInt:[cardDict objectForKey:kCardId]]
                                          name:[cardDict objectForKey:kCardName]
                                          type:[self convertStringToInt:[cardDict objectForKey:kCardType]]];
    return card;
}

I don't understand the difference between these two methods (besides that one is random)--I don't understand why different CRGCard init methods are called.

Why can't we just do:

- (CRGCard *)randomCardFromCardList {
    return [self cardWithID:arc4random_uniform(_listOfAllCards.count)];
}

-(int) convertStringToInt:(NSString *)string {
    return [string intValue];
}

This method is really only acceptable if we have some special way of converting. Like if we've got some crazy logic for determining if this string is actually hex or binary rather than decimal, and no matter what form the number is, we return an int value.

If the idea is to increase readability, this is completely redundant. intValue is a common method across several Objective-C classes. NSString, NSMutableString, NSNumber, and NSDecimalNumber all respond to intValue, additionally anyone who writes any custom class that might store some sort of values will most likely write an intValue method for their class too. And honestly, it doesn't matter what type the receiver is... we only care that we need an intValue out of it, and that's readable enough.

But there's another problem here too. This is a useless instance method. If we really need a special way of converting from strings to ints, there are two better options.

  1. An NSString category, where we write an instance method for NSString objects
  2. A C-Style function, which would look something like this:

    int stringToInt(NSString *string) {
        // do something hopefully more than simply:
        return [string intValue];
    }
    

Between the two, the first option is probably the best.


#pragma mark - Diagnostic Methods
-(void) logCards {
    for (NSDictionary *dict in self.listOfAllCards) {
        for (id key in dict) {
            NSString *string = [dict objectForKey:key];
            NSLog(@"%@", string);
        }
    }
}

Given this is in a section of code marked as Diagnostic Methods, and really should only be used in debug builds, let's ensure this code is only executed in debug builds. We don't want to eliminate the entire method, because if someone left a call to logCards in a release build, eliminating the method would cause problems, so let's just #if DEBUG the body:

#pragma mark - Diagnostic Methods
-(void) logCards {
#if DEBUG
    for (NSDictionary *dict in self.listOfAllCards) {
        for (id key in dict) {
            NSString *string = [dict objectForKey:key];
            NSLog(@"%@", string);
        }
    }
#endif
}
share|improve this answer

Card.h/Card.m

This is a really big question, so I'm going to address one thing at a time...


@property int cardId;
@property NSString* name;
@property CardType type;

@property int unitStrength;
@property int unitHealth;
@property int abilitySlots;

It's possible/probable that these should all be readonly. Does it make sense to change any of these properties after we've instantiated the object from the plist (or wherever we're pulling the data to generate the cards from)?


-(instancetype) initWithId:(int)cardId 
                      name:(NSString *)name 
                      type:(CardType)type 
              unitStrength:(int)unitStrength 
                unitHealth:(int)unitHealth 
              abilitySlots:(int)abilitySlots {
    self = [super init];
    if (self) {
        _cardId = cardId;
        _name = name;
        _type = type;
        _unitStrength = unitStrength;
        _unitHealth = unitHealth;
        _abilitySlots = abilitySlots;
    }
    return self;
}

-(instancetype) initWithId:(int)cardId name:(NSString *)name type:(CardType)type {
    self = [super init];
    if (self) {
        _cardId = cardId;
        _name = name;
        _type = type;
    }
    return self;
}

We shouldn't repeat ourselves and we shouldn't leave uninitialized values.

Let's replace the latter with:

- (instancetype) initWithId:(int)cardId name:(NSString *)name type:(CardType)type {
    return [self initWithId:cardId 
                       name:name 
                       type:type 
               unitStrength:0 
                 unitHealth:0
               abilitySlots:0];
}
share|improve this answer

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.