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.

I have a card matching game with a model of:

  • Card
  • Deck
  • PlayingCard
  • PlayingCardDeck

And another model called CardMatchingGame.

Now, I had a task to add a button to the view for new game, which deals the cards again and setting the labels to 0 (#flips and score).

I added this method:

- (IBAction)newGame:(UIButton *)sender {

    self.flipsCount = 0;
    self.game = nil;
    for (UIButton *button in self.cardButtons) {
        Card *card = [self.game cardAtIndex:[self.cardButtons indexOfObject:button]];
        card.unplayble = NO;
        card.faceUp = NO;
        button.alpha = 1;
    }

    self.notificationLabel.text = nil;
    [self updateUI];
}

And this is the whole viewcontroller:

#import "CardGameViewController.h"
#import "PlayingCardsDeck.h"
#import "CardMatchingGame.h"


@interface CardGameViewController ()

@property (weak, nonatomic) IBOutlet UILabel *flipsLabel;
@property (weak, nonatomic) IBOutlet UILabel *notificationLabel;
@property (weak, nonatomic) IBOutlet UILabel *scoreCounter;
@property (strong, nonatomic) IBOutletCollection(UIButton) NSArray *cardButtons;

@property (strong, nonatomic) CardMatchingGame *game;
@property (nonatomic) int flipsCount;

@end


@implementation CardGameViewController

//creating the getter method that creates a new card game.
-(CardMatchingGame *) game {

    if (!_game) _game = [[CardMatchingGame alloc] initWithCardCount:self.cardButtons.count usingDeck:[[PlayingCardsDeck alloc] init]];
    return _game;
}

//creating a setter for the IBOutletCollection cardButtons
-(void) setCardButtons:(NSArray *)cardButtons {

    _cardButtons = cardButtons;
   [self updateUI];
}


//creating the setter for the flipCount property. Whick is setting the flipsLabel to the right text and adding the number of counts.
-(void) setFlipsCount:(int)flipsCount {

    _flipsCount = flipsCount;
    self.flipsLabel.text = [NSString stringWithFormat:@"Flips: %d", self.flipsCount];

}


-(void) updateUI {

    for (UIButton *cardButton in self.cardButtons) {
        Card *card = [self.game cardAtIndex:[self.cardButtons indexOfObject:cardButton]];
        [cardButton setTitle:card.contents forState:UIControlStateSelected];
        [cardButton setTitle:card.contents forState:UIControlStateSelected|UIControlStateDisabled];
        cardButton.selected = card.isFaceUp;
        cardButton.enabled = !card.unplayble;
        if (card.unplayble) {
            cardButton.alpha = 0.1;
        }

        //updating the score 
        self.scoreCounter.text = [NSString stringWithFormat:@"Score: %d", self.game.score];

        //if notification in CardMatchingGame.m is no nil, it will be presented 
        if (self.game.notification) {

        self.notificationLabel.text = self.game.notification;

        }
    }
}

//Here I created a method to flipCards when the card is selected, and give the user a random card from the deck each time he flips the card. After each flip i'm incrementing the flipCount setter by one.
- (IBAction)flipCard:(UIButton *)sender {

    [self.game flipCardAtIndex:[self.cardButtons indexOfObject:sender]];
    self.flipsCount++;
    [self updateUI];
}


//cleaning the history using the cleanHistory method in CardMatchingGame.m and creating a new game by setting geme to nil
- (IBAction)newGame:(UIButton *)sender {

    self.flipsCount = 0;
    self.game = nil;
    for (UIButton *button in self.cardButtons) {
        Card *card = [self.game cardAtIndex:[self.cardButtons indexOfObject:button]];
        card.unplayble = NO;
        card.faceUp = NO;
        button.alpha = 1;
    }

    self.notificationLabel.text = nil;
    [self updateUI];
}


@end

Could you please tell me if this is a logical solution? I'm a very new programmer and critiques are important to me.

share|improve this question
    
if not Code Review, try at Stackoverflow or GameDev.stackexchange.com –  Timothy Groote Mar 12 '13 at 9:38
    
JohnBigs - I have flagged your question for mod review to see if this would be a good candidate for codereview. If it is, they will automatically migrate the question for you. You are free to flag your question and request the migrate as well. –  GlenH7 Mar 12 '13 at 11:03
    
@TimothyGroote - this wouldn't be on-topic for SO as there isn't a problem. This may be too low level of a question to fit in at Gamedev. Codereview is likely the best fit. –  GlenH7 Mar 12 '13 at 11:06
    
@GlenH7 thank you glenn –  nir Mar 12 '13 at 11:30
    
What is a Card vs. a PlayingCard? Are there Card objects (in your game) that are not PlayingCard instances? –  Nate Apr 20 '13 at 22:05
add comment

migrated from programmers.stackexchange.com Mar 12 '13 at 11:53

This question came from our site for professional programmers interested in conceptual questions about software development.

1 Answer

You certainly get points for using IBOutletCollection(UIButton). But, it's unclear why you want to replace this array with a new one? If you're going to do that it should be this view controller that creates the new buttons and adds them as subviews. Also, if you're going to create the buttons in code then I wouldn't expect the first set to be defined in the XIB.

I'm assuming (based on your property definitions) that you're using ARC too.

This section of code can be better by maintaining and incrementing an index instead of interrogating the array you're looping over for the index:

for (UIButton *cardButton in self.cardButtons) {
    Card *card = [self.game cardAtIndex:[self.cardButtons indexOfObject:cardButton]];

Also, in the updateUI button loop you don't need to set the title multiple times to the same thing and the loop should be completed before you modify the label text.

You want to know about logic, but there is basically no logic in this class. What this class does have is a bit of confusion about whether it owns the buttons and associated cards or if some other class does. The only comments that can be made really relate to efficiency rather than logic.

share|improve this answer
add comment

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.