2
\$\begingroup\$

I'm learning iOS through this past Stanford CS193P course. I've just completed the first assignment and would love a code review as I can't actually turn in an assignment for feedback since the course has already completed.

The requirements can be found here but any suggestions on best practices and my implantation would be helpful. The app works and is in a state I would submit if I could. I'm just looking for another set of eyes.

GitHub

    import Foundation
    /**
     a game card that contains the following var:
     - isFaceUp
     - isMatched
     - identifier
     */
    struct Card: Hashable
    {
        var hashValue: Int {
            return identifier
        }

        static func == (lhs: Card, rhs: Card) -> Bool {
            return lhs.identifier == rhs.identifier
        }

        var isFaceUp = false
        var isMatched = false
        private var identifier: Int

        private static var identifierFactory = 0

        private static func getUniqueIdentifer() -> Int {
            identifierFactory += 1
            return identifierFactory
        }

        init(){
            self.identifier = Card.getUniqueIdentifer()
        }
    }

        import Foundation

    struct Concentration
    {
        private(set) var cards = [Card]()
        private(set) var score = 0
        private(set) var flipCount = 0

        private var indexOfOneAndOnlyFaceUp: Int? {
            get {
                return cards.indices.filter({cards[$0].isFaceUp}).oneAndOnly
            }

            set {
                for flipDownIndex in cards.indices {
                    cards[flipDownIndex].isFaceUp = (flipDownIndex == newValue)
                }
            }
        }

        private var selectedIndex = Set<Int>()
        private var lastIndexWasSelected = false
        /// returns true if all cards have been matched
        var allCardsHaveBeenMatched: Bool {
            for index in cards.indices {
                if !cards[index].isMatched { return false }
            }
            return true
        }

        /**
         Choose a card at an index.
         Handles flip count, if a card is faced up, and matching of cards
         */
        mutating func chooseCard(at index: Int){
            assert(cards.indices.contains(index), "Concentration.chooseCard(at:\(index)): index is not in the cards")
            let cardWasPreviouslySelected = selectedIndex.contains(index)
            if !cards[index].isMatched {
                // only flip cards that are visible
                flipCount += 1
                if let matchIndex = indexOfOneAndOnlyFaceUp, matchIndex != index {
                    // 2 cards are face up, check if cards match
                    if cards[index] == cards[matchIndex] {
                        cards[index].isMatched = true
                        cards[matchIndex].isMatched = true
                        if lastIndexWasSelected {
                            // add extra to account for subtracting earlier
                            score += 3
                        } else {
                            score += 2
                        }
                    }else {
                        // no match
                        if cardWasPreviouslySelected {score -= 1}
                    }
                    cards[index].isFaceUp = true
                } else {
                    // one card is selected, turn down other cards and set this card
                    if cardWasPreviouslySelected { score -= 1 }
                    indexOfOneAndOnlyFaceUp = index
                    lastIndexWasSelected = cardWasPreviouslySelected
                }
            }

            selectedIndex.insert(index)
        }

        init(numberOfPairsOfCards: Int){
            assert(numberOfPairsOfCards > 0, "Concentraation.init(numberOfPairsOfCards:\(numberOfPairsOfCards) you must have multiple pairs of cards")
            for _ in 0..<numberOfPairsOfCards {
                let card = Card()
                cards += [card, card]
            }
            shuffleCards()
        }

        mutating private func shuffleCards() {
            for _ in 0..<cards.count {
                // sort seems better than .swap()
                cards.sort(by: {_,_ in arc4random() > arc4random()})
            }
        }
    }

    extension Collection {
        var oneAndOnly: Element? {
            return count == 1 ? first : nil
        }
    }

import Foundation

class Theme
{
    enum Theme: UInt32 {
        case halloween
        case love
        case animal
        case waterCreatures
        case plants
        case weather
    }

    /// get an array of icons by theme
    func getThemeIcons(by theme: Theme) -> [String] {
        switch theme {
        case .halloween:
            return ["πŸ‘»", "😈", "πŸ‘Ή", "πŸ‘Ί", "πŸŽƒ", "☠️", "πŸ¦‡", "πŸ’€"]
        case .love:
            return ["πŸ’•","πŸ’‹", "πŸ’Œ", "πŸ’˜", "❀️", "πŸ’“", "πŸ’–", "πŸ’”"]
        case .animal:
            return ["🐢", "πŸ’", "πŸ”", "🦊", "πŸ¦‰", "🐭", "πŸ₯", "🦎"]
        case .waterCreatures:
            return ["πŸ¦‘", "πŸ™", "🐠", "🐑", "🐳", "🐚", "πŸ¦€", "🦈"]
        case .plants:
            return ["🍁", "πŸ„", "🌹", "πŸ’", "🌾", "🌸", "🌺", "🌻"]
        case .weather:
            return ["πŸ”₯", "❄️", "β˜€οΈ", "πŸ’¦", "β˜”οΈ", "🌬", "☁️", "πŸŒͺ", "β›ˆ"]
        }
    }

    private func random() -> Theme {
        let max = Theme.weather.rawValue
        let randomIndex = arc4random_uniform(max + UInt32(1))
        return Theme(rawValue: randomIndex) ?? Theme.halloween
    }

    /**
     get a random array of themed icons

     - Author:
     Anna
     */
    func getRandomThemeIcons() ->[String] {
        return getThemeIcons(by: random())
    }

}

import UIKit

class ViewController: UIViewController {

    private lazy var game = Concentration(numberOfPairsOfCards: numberOfPairsOfCards)

    var numberOfPairsOfCards: Int {
        return (cardButtons.count + 1) / 2
    }

    @IBOutlet weak var finishedLabel: UILabel!
    @IBOutlet weak var scoreCountLabel: UILabel!
    @IBOutlet weak var flipCountLabel: UILabel!

    private var flipCount = 0 {
        didSet {
            flipCountLabel.text = "Flip Count: \(flipCount)"
        }
    }

    private var scoreCount = 0 { didSet { scoreCountLabel.text = "Score: \(scoreCount)"} }

    @IBOutlet var cardButtons: [UIButton]!

    @IBAction func touchCard(_ sender: UIButton) {
        if let cardNumber = cardButtons.index(of: sender){
            game.chooseCard(at: cardNumber)
            updateViewFromModel()
        }else {
            print("card is not in cardButton array")
        }
    }

    @IBAction func touchNewGame(_ sender: UIButton) {
        // reset game
        game = Concentration(numberOfPairsOfCards: (cardButtons.count + 1) / 2)
        // reset theme choices
        emojiChoices = theme.getRandomThemeIcons()
        // update view
        updateViewFromModel()
    }

    private func updateViewFromModel(){
        flipCount = game.flipCount
        scoreCount = game.score
        for index in cardButtons.indices {
            let button = cardButtons[index]
            let card = game.cards[index]
            if card.isFaceUp {
                button.setTitle(emoji(for: card), for: UIControlState.normal)
                button.backgroundColor = #colorLiteral(red: 1, green: 1, blue: 1, alpha: 1)
            } else {
                button.setTitle("", for: UIControlState.normal)
                button.backgroundColor = card.isMatched ? #colorLiteral(red: 1, green: 1, blue: 1, alpha: 0) : #colorLiteral(red: 1, green: 0.5763723254, blue: 0, alpha: 1)
            }
        }
        finishedLabel.textColor = game.allCardsHaveBeenMatched ? #colorLiteral(red: 1, green: 1, blue: 1, alpha: 1) : #colorLiteral(red: 0.9999960065, green: 1, blue: 1, alpha: 0)
        finishedLabel.text = game.score >= 0 ? "Nice work! πŸ‘" : "Phew, 🐻ly made it"
    }
    private var theme = Theme()

    private lazy var emojiChoices = theme.getRandomThemeIcons()

    private var emoji = [Card: String]()

    private func emoji(for card: Card) -> String {
        if emoji[card] == nil, emojiChoices.count > 0 {
            emoji[card] = emojiChoices.remove(at: emojiChoices.count.arc4random)
        }
        return emoji[card] ?? "?"
    }
}

extension Int {
    var arc4random: Int {
        if self > 0 {
            return Int(arc4random_uniform(UInt32(self)))
        } else if self < 0 {
            return -Int(arc4random_uniform(UInt32(abs(self))))
        }else {
            return 0
        }
    }
}
\$\endgroup\$
4
  • \$\begingroup\$ why would this be down voted?? pretty unfriendly welcoming. :( \$\endgroup\$ May 24, 2018 at 20:10
  • 1
    \$\begingroup\$ Welcome to Code Review! Can you edit your post to include more details about this Memory game? If possible, please include the assignment/requirements. \$\endgroup\$ May 24, 2018 at 20:18
  • \$\begingroup\$ @SamOnela I can link to a PDF but I have a feeling that won't fly here either. I was looking for best practices and general comments on my implementation because the app works. drive.google.com/drive/folders/… \$\endgroup\$ May 24, 2018 at 20:26
  • 2
    \$\begingroup\$ Welcome to Code Review! Links can rot. Please include a description of the challenge here in your question. \$\endgroup\$ May 24, 2018 at 22:44

1 Answer 1

3
\$\begingroup\$

Concentration.swift

And I have few notes:

  1. It's better to describe what this entity is about. You can add comment above declaration.
import Foundation

struct Concentration
{
    private(set) var cards = [Card]()
    private(set) var score = 0
  1. This code can be rewritΠ΅en:
var allCardsHaveBeenMatched: Bool {
        for index in cards.indices {
            if !cards[index].isMatched { return false }
        }
        return true
    }

better/shorter version:

var allCardsHaveBeenMatched: Bool { !cards.contains(where: { !$0.isMatched } }
  1. This method is doing too much and it's hard to read it, consider to spit it into few smaller methods (Single Responsibility Principle).
/**
     Choose a card at an index.
     Handles flip count, if a card is faced up, and matching of cards
     */
mutating func chooseCard(at index: Int){

Card.swift

  1. Use UUID() instead
private static var identifierFactory = 0

    private static func getUniqueIdentifer() -> Int {
        identifierFactory += 1
        return identifierFactory
    }

Theme.swift

Can be rewritten like this (Using enum without enclosing class):

import Foundation


enum Theme: UInt32 {
    case halloween
    case love
    case animal
    case waterCreatures
    case plants
    case weather

    /// get an array of icons by theme
    var icons: [String] {
        switch self {
        case .halloween:
            return ["πŸ‘»", "😈", "πŸ‘Ή", "πŸ‘Ί", "πŸŽƒ", "☠️", "πŸ¦‡", "πŸ’€"]
        case .love:
            return ["πŸ’•","πŸ’‹", "πŸ’Œ", "πŸ’˜", "❀️", "πŸ’“", "πŸ’–", "πŸ’”"]
        case .animal:
            return ["🐢", "πŸ’", "πŸ”", "🦊", "πŸ¦‰", "🐭", "πŸ₯", "🦎"]
        case .waterCreatures:
            return ["πŸ¦‘", "πŸ™", "🐠", "🐑", "🐳", "🐚", "πŸ¦€", "🦈"]
        case .plants:
            return ["🍁", "πŸ„", "🌹", "πŸ’", "🌾", "🌸", "🌺", "🌻"]
        case .weather:
            return ["πŸ”₯", "❄️", "β˜€οΈ", "πŸ’¦", "β˜”οΈ", "🌬", "☁️", "πŸŒͺ", "β›ˆ"]
        }
    }

    static func randomTheme() -> Theme {
        let max = Theme.weather.rawValue
        let randomIndex = arc4random_uniform(max + UInt32(1))
        return Theme(rawValue: randomIndex) ?? Theme.halloween
    }
}

To summarize: your code is not bad, but need some improvements. You are using encapsulation to hide implementation details and prefer values type (like structs) over reference types (classes). It's good stating point!

PS: Welcome to Code Review!!

\$\endgroup\$
3
  • 1
    \$\begingroup\$ Your suggestion for var allCardsHaveBeenMatched is shorter, but not better: It always traverses the entire array, even if a non-match has been found (it does not β€œshort-circuit”). – Better use contains instead of reduce. \$\endgroup\$
    – Martin R
    May 30, 2018 at 1:17
  • \$\begingroup\$ How does contains short-circuit? \$\endgroup\$ May 30, 2018 at 14:40
  • \$\begingroup\$ It will return once first not matched card will be found \$\endgroup\$
    – Andrew
    May 30, 2018 at 14:42

Your Answer

By clicking β€œPost Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.