3
\$\begingroup\$

I've starting writing the card game Uno the past couple of days and was wondering if you can review my code please. The initialization is done I think, a couple of game functions already exist, please have look.

Please give me feedback concerning, structure, logic, data structures, naming conventions, anything you're noticing!

I want to make sure everything is set until I move on to the View and Controller.

import UIKit
import Foundation

// Represents all jokers of the game.
enum Joker: String, CaseIterable {
    // Only four copies of each will be created
    case joker, drawFour, swapCircle
    func points() -> Int {
        switch self {
        case .joker, .drawFour, .swapCircle: return 50
        } // returns points of each card
    }
}

// Represents all "normal" cards.
enum Value: String, CaseIterable {
    // A cartesian product with all colors will be built.
    case one, two, three, four, five, six, seven, eight, nine, drawTwo, skip, turn
    // returns points of each card
    func points() -> Int {
        switch self {
        // THIS DOES NOT LOOK VERY NICE. IS THERE A SMOOTHER WAY?
        case .one: return 1
        case .two: return 2
        case .three: return 3
        case .four: return 4
        case .five: return 5
        case .six: return 6
        case .seven: return 7
        case .eight: return 8
        case .nine: return 9
        case .drawTwo, .skip, .turn: return 20
        }
    }
}

// Represents all colors.
enum Color: String, CaseIterable {
    case red, blue, yellow, green
}

// Represents the structure of a card.
struct Card {
    let value: Value? // is nil when card is a joker
    let color:  Color? // is nil when card is a joker
    let joker:  Joker? // is nil when card has a value and color
}

// Represents the two stacks of the game. Game stack as well as draw stack will be instances of stack, depending on which constructor is called the correct instance will be created.
class Stack {
    public var stack: [Card]

    // Creates the draw when the game is first initalized.
    private static func createDrawStack(with amountOfCards: Int) -> [Card] {
        var cards = [Card]()
        // add 2 cards of each value
        for color in Color.allCases {
            for value in Value.allCases {
                for _ in 0..<amountOfCards { // add two copies
                    cards += [Card(value: value, color: color, joker: nil)]
                }
            }
        }
        for _ in 0..<(amountOfCards * 2) {
            // add 1 swap-circle
            cards += [Card(value: nil, color: nil, joker: .swapCircle)]
            // add 1 draw-four
            cards += [Card(value: nil, color: nil, joker: .drawFour)]
            // add 1 joker
            cards += [Card(value: nil, color: nil, joker: .joker)]
        }
        return cards.shuffled() // TODO: In-place shuffling wouldn't work, maybe not shuffling in here is smoother
    }

    // Creates the game stack when the game is first initalized.
    private static func createGameStack(with drawStack: Stack) -> [Card] {
        return [drawStack.stack.popLast()!]
    }

    init(createDrawStackWith amount: Int) {
        self.stack = Stack.createDrawStack(with: amount)
    }
    init(createGameStackWith drawStack: Stack) {
        self.stack = Stack.createGameStack(with: drawStack)
    }
}

// Represents a player and the actions a player can do.
class Player {
    let id: Int
    var hand = [Card?]()
    var didDraw = false

    // Receives index of button pressed and returns the card selected, so that cardsMatch() can check if it's a valid turn. If so, playCard() will be called.
    public func wantsToPlayCard(at index: Int) -> Card {
        return hand[index]!
    }

    // Moves card requested from user hand to playStack
    public func playCard(at index: Int, onto playStack: Stack) {
        if let card = self.hand.remove(at: index) {
            playStack.stack.append(card)
        }
    }

    // Moves a card from drawStack to user hand
    public func drawCard(from deck: Stack) {
        self.hand.append(deck.stack.popLast()!)
    }

    init(withID id: Int) {
        self.id = id
    }
}

// Represents the whole game. This is the core of the game.
class Uno {

    public var drawStack: Stack // creates a stack to draw from with 2 copies of each color
    public var playStack: Stack // creates a stack where the game will be played
    public var players: [Player] // will be initialized dependent of amount

    // Moves play stack back into draw stack.
    public func resetDrawStack() {
        let topCard = playStack.stack.popLast()! // save top card
        drawStack.stack = playStack.stack // move rest of playStack back into drawStack
        drawStack.stack.shuffle()
        playStack.stack.removeAll()
        playStack.stack += [topCard]
    }

    // Checks if a requested card can be played or not.
    public func cardsMatch(compare topCard: Card, with handCard: Card) -> Bool {
        // if colors match
        if let colorTopCard = topCard.color, let colorHandCard = handCard.color {
            if colorTopCard == colorHandCard {
                return true
            }
        }
        // if values match
        if let valueTopCard = topCard.value, let valueHandCard = handCard.value {
            if valueTopCard == valueHandCard {
                return true
            }
        }
        // if its a joker
        if let _ = handCard.joker {
            return true
        }
        // else
        return false
    }

    // Deals cards to players at the start of round.
    private func dealCards(with amountOfCards: Int) {
        for id in players.indices {
            for _ in 0..<amountOfCards {
                players[id].drawCard(from: drawStack)
            }
        }
    }

    // Creates the players.
    private static func createPlayers(with amountOfPlayers: Int) -> [Player] {
        var players = [Player]()
        for id in 0..<amountOfPlayers {
            players += [Player(withID: id)]
        }
        return players
    }

    // Constructs game dependent on settings.
    init(amountOfPlayers: Int, amountOfCopies: Int, amountOfCards: Int) {
        self.drawStack = Stack(createDrawStackWith: amountOfCopies)
        print(self.drawStack.stack)
        self.playStack = Stack(createGameStackWith: self.drawStack)
        self.players = Uno.createPlayers(with: amountOfPlayers) // creates 2 players
        self.dealCards(with: amountOfCards)
    }
}

var game = Uno(amountOfPlayers: 2, amountOfCopies: 2, amountOfCards: 7)
\$\endgroup\$
  • 1
    \$\begingroup\$ This isn’t as constructive as most other comments might be, but consider a pun in the title. Something like: Creating a Swift game of Uno or whatever you think is cool. \$\endgroup\$ – Grant Garrison Dec 12 '18 at 0:27
2
\$\begingroup\$

I don't much like your Card struct. What would you do if you found a card that had a non-nil value for all three properties? It's better to make impossible things impossible.

enum Card {
    case suit(Value, Color)
    case special(Joker)
}

With the above, there is no way to make a card that has a non-nil Joker as well as non-nil Value. It also requires every card that has a Value to also have a Color which is something else that your current struct doesn't enforce.


As a general rule, if a function doesn't require self, it shouldn't be a method. At minimum it should be static, IMO it really should be a free function (one written outside of any class.) This rule applies to your cardsMatch(compare:with:) method. Either define it as static (as in public static func cardsMatch...) or write it outside of the class.

Also, if you used my Card idea above, the match method would become considerably simpler.

public func cardsMatch(compare topCard: Card, with handCard: Card) -> Bool {
    switch (topCard, handCard) {
    case (_, .special):
        return true
    case let (.suit(topValue, topColor), .suit(handValue, handColor)):
        return topValue == handValue || topColor == handColor
    }
}

Note, I wrote the above as a free function.

I think the above exercise also exposed a logic error in the function. Can't you lay any card on top of a special/joker card?

\$\endgroup\$
  • \$\begingroup\$ Thank you a lot! I will think about it later and come back to you :-) \$\endgroup\$ – Marcel B Dec 12 '18 at 12:20
  • \$\begingroup\$ I added to my response. \$\endgroup\$ – Daniel T. Dec 12 '18 at 12:44
  • \$\begingroup\$ Thx! No, I should've added my assumptions. A joker will stay black in the UI but the internal color will be whatever color they chose. That will be in case draw-four and joker (both have wish-a-color). If someones plays a swap-circle, swap circle will adapt the color of the card that it was put on. Never mentioned this, sorry. That makes your solution for the card not so optimal, does it? \$\endgroup\$ – Marcel B Dec 12 '18 at 14:51
  • \$\begingroup\$ Also, why would I write a free func? I won't need to compare cards outside of game. I always thought like this: comparing two cards is the games job, so it should be a method of the game. Same with anything that mutates anything. The one that's got the job will have the method. Is this wrong? \$\endgroup\$ – Marcel B Dec 12 '18 at 14:54
  • \$\begingroup\$ I think you can agree that it's generally bad form to pass parameters into a function that the function doesn't need. For example you don't pass a player or stack into the compare function. Well self is a parameter too. If you don't use it, you shouldn't pass it in. After all, I can show you two cards and ask if one can stack on top of the other without bringing along an entire deck and dealing cards out, but your function requires all of that because it requires self. This means you should either making the function static or a free function. \$\endgroup\$ – Daniel T. Dec 12 '18 at 15:04
1
\$\begingroup\$

Imports

If you're importing UIKit, you don't need to import Foundation. Foundation is included in the UIKit import.

Classes

The underlying class variables in all of the classes are public. In most of these cases, the methods that populate them are private. That means that the vars should most likely also be private, or at the very least private (set) if you wanted to be able to still view the contents of them.

Additionally there are a lot of public methods that seem like they are unnecessarily public. When making a method or property public, you should ask yourself if it is necessary to access that value outside of the scope of your project. If not, then you're probably safe to use internal (the default).

Card

I believe Daniel T covered the Card struct in a great way. I would agree with his suggestions regarding that. Removing the unnecessary optionals and setting it up in an enum as he did would help quite a bit.

Stack

I am not a fan of the name stack for the array of Card contained in the Stack. It could use a more descriptive name. I would probably name it cards because it makes it more obvious and less redundant when you're doing things like drawStack.stack.popLast() as drawStack.cards.popLast().

In the createGameStack method, you force unwrap the initial draw and this should be fine. However if something were to go wrong, this would result in a crash. You could perhaps add a guard here.

private static func createGameStack(with drawStack: Stack) -> [Card] {
    guard let topCard = drawStack.stack.popLast() else {
        print("Why is the drawStack empty?")
        return []
    }

    return [topCard]
}

The initializers are kind of redundant here. You have static methods to populate the stack based on the type that you're creating, but you also have corresponding initializers. I would consider a generic initializer that would set the stack to an empty array, and then have your createDeck methods become public and return a Stack object. Finally I would update your createGameStack method to be public and not static. This is dependent on a deckStack existing, so it makes sense for it to be a method on that object.

init() {
    self.cards = [] // This could also be done as public var cards: [Card] = []
}

static func createDrawStack(with amountOfCards: Int) -> Stack {
    let drawStack = Stack()
    // Create the card array here.
    drawStack.cards = cards.shuffled()
    return drawStack
}

func createGameStack() -> Stack {
    let gameStack = Stack()
    guard let topCard = drawStack.cards.popLast() else {
        print("Why is the drawStack empty?")
        return gameStack
    }

    gameStack.cards = [topCard]
    return gameStack
}

This gives you the implementation like this:

self.drawStack = Stack.createDrawStack(with: amountOfCopies)
self.playStack = drawStack.createGameStack()

Player

The hand does not need to be an array of optional cards. The player should fit into one of two categories; 1. they have cards in hand or 2. they have no hand and have won the game. In both cases there isn't a situation where the user will have something but possibly nothing in their hand, which is what an optional card would represent.

The wantsToPlayCard(at index:) method returns a Card, but does not perform any overflow checking. This as well as the playCard(at index: onto playStack:) method should both check to make sure that the index is not out of the bounds of the array at a minimum.

The playCard(at index: onto playStack:) method could (should) be split into two methods, one in the player class and one in the stack class. You want to enforce separation of concerns here, and due to the way you have the method laid out, you have a direct dependency on something that the player should not. When separating the concerns here, you would want the Player.playCard method to identify the card which will be played, which you are already in the wantsToPlayCard(index:) method. Then in the stack there would be a play(_ card: Card) method that would do the append on the stack.

Doing this enables you to remove the player's dependency on the stack and implement something like this playStack.play(currentPlayer.wantsToPlayCard(at: 4)).

var didDraw is unused.

Similar to the above mentioned separation of concerns, drawCard(from deck:) seems like is should be added to the stack class and the player's implementation should be more like drawCard(_ card: Card) where you are provided a card from the deck and merely adding it to the player's hand.

Uno

I'll have to come back and take a look at the Uno class, but I feel this is a good start for suggestions and ideas.

\$\endgroup\$
  • \$\begingroup\$ Thanks, it certainly is. Will come back to you. :-) \$\endgroup\$ – Marcel B Dec 12 '18 at 19:26

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.