2
\$\begingroup\$

So I'm working through a MOOC, Stanford's "Developing iOS 11 Apps with Swift", and in the course the professor highly emphasizes how important it is to separate your View from your Model in the MVC architecture. One of the assignments is a simple implementation of this game: Set. While I did decouple them, it feels off. I was looking for advice on how to improve the model structure a bit more, without adding in specific view logic. For example, when I started out, instead of using Ints for the fields in my Card model, I used enums like this:

enum Symbol {
  case square, circle, triangle
}

Which worked somewhat well but the professor advised against doing this because we would be updating the View layer to use different symbols. Here is the code I ended up using, but as I mentioned above it feels off and was wondering how someone more experienced would approach this (The most relevant part is Card.swift but I included everything to illustrate how it was being used):

Card.swift

import Foundation

struct Card : Hashable {
    var hashValue: Int {
        return self.identifier
    }

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

    let symbol: Int
    let color: Int
    let shading: Int
    let quantity: Int

    private let identifier: Int
    private static var previousIdentifier = 0
    private static func generateUid() -> Int {
        previousIdentifier += 1
        return previousIdentifier
    }

    init(symbol: Int, color: Int, shading: Int, quantity: Int) {
        self.symbol = symbol
        self.color = color
        self.shading = shading
        self.quantity = quantity
        self.identifier = Card.generateUid()
    }

    static func doesMakeSet(_ cards: [Card]) -> Bool {
        let colors = cards[0].color == cards[1].color && cards[1].color == cards[2].color
        let shading = cards[0].shading == cards[1].shading && cards[1].shading == cards[2].shading
        let quantities = cards[0].quantity == cards[1].quantity && cards[1].quantity == cards[2].quantity
        let symbols = cards[0].symbol == cards[1].symbol && cards[1].symbol == cards[2].symbol
        return colors || shading || quantities || symbols
    }
}

Set.swift

import Foundation

class Set {
    private var deck = [Card]()
    private(set) var board = [Card]() {
        didSet {
            assert(board.count <= 24, "Set.board: board cannot contain more than 24 cards")
        }
    }
    var canDealMoreCards: Bool {
        let matchesThatCanBeRemoved = board.filter { matchedCards.contains($0) }
        let occupiedSpacesOnBoard = board.count - matchesThatCanBeRemoved.count
        let freeSpaces = 24 - occupiedSpacesOnBoard
        let cardsRemaining = deck.count
        return cardsRemaining != 0 && freeSpaces >= 3
    }

    private(set) var selectedCards = [Card]()
    private(set) var matchedCards = [Card]()
    var currentSelectionIsMatch: Bool?

    private(set) var score = 0

    private func createDeck() -> [Card] {
        var newDeck = [Card]()
        let noOfOptions = 1...3
        for quantity in noOfOptions {
            for symbol in noOfOptions {
                for color in noOfOptions {
                    for shading in noOfOptions {
                        newDeck.append(Card(symbol: symbol, color: color, shading: shading, quantity: quantity))
                    }
                }
            }
        }
        return newDeck
    }

    func dealCards(numberOfCards: Int = 3) {
        if numberOfCards + board.count > 24 {
            board = board.filter { !matchedCards.contains($0) }
        }
        if deck.count >= numberOfCards {
            let range = 0..<numberOfCards
            board.append(contentsOf: deck[range])
            deck.removeSubrange(range)
            assert(board.count <= 24, "Set.dealCards(): there can't be more than 24 cards on the board but \(board.count) are")
        }
    }

    func cardChosen(index: Int) {
        if index > board.count - 1 {
            return
        }
        let card = board[index]
        if matchedCards.contains(card) {
            return
        }
        if selectedCards.count == 3 {
            if Card.doesMakeSet(selectedCards) {
                matchedCards += selectedCards
                selectedCards.removeAll()
                score += 3
                currentSelectionIsMatch = true
                dealCards()
            } else {
                currentSelectionIsMatch = false
                selectedCards.removeAll()
                score -= 5
            }
        } else {
            currentSelectionIsMatch = nil
        }
        if selectedCards.contains(card) {
            selectedCards.remove(at: selectedCards.index(of: card)!)
        } else {
            if !matchedCards.contains(card) {
                selectedCards.append(card)
            }
        }
    }

    init() {
        deck = createDeck()
        var shuffled = [Card]()
        for _ in deck.indices {
            let randomIndex = deck.count.arc4random
            shuffled.append(deck.remove(at: randomIndex))
        }
        deck = shuffled
        assert(deck.count == 81)
        dealCards(numberOfCards: 12)
    }
}

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
        }
    }
}

ViewController.swift

import UIKit

class ViewController: UIViewController {

    var game = Set()

    @IBOutlet var cardButtons: [UIButton]!
    @IBOutlet weak var deal3CardsButton: UIButton!
    @IBOutlet weak var scoreLabel: UILabel!
    @IBOutlet weak var statusLabel: UILabel!

    override func viewDidLoad() {
        updateViewFromModel()
    }

    @IBAction func cardTouched(_ sender: UIButton) {
        if let index = cardButtons.index(of: sender) {
            game.cardChosen(index: index)
            updateViewFromModel()
        }

    }

    @IBAction func deal3CardsTouched() {
        game.dealCards()
        updateViewFromModel()
    }

    @IBAction func newGameTouched() {
        game = Set()
        updateViewFromModel()
    }

    func updateViewFromModel() {
        scoreLabel.text = "Score: \(game.score)"
        if let match = game.currentSelectionIsMatch {
            statusLabel.text = match ? "Match!" : "Not a Match!"
        } else {
            statusLabel.text = ""
        }
        cardButtons.forEach {
            $0.hide()
        }
        for index in game.board.indices {
            let text = getAttributedString(forCard: game.board[index])
            cardButtons[index].setAttributedTitle(text, for: UIControlState.normal)
            if game.selectedCards.contains(game.board[index]) {
                cardButtons[index].backgroundColor = #colorLiteral(red: 0.8039215803, green: 0.8039215803, blue: 0.8039215803, alpha: 1)
            } else if game.matchedCards.contains(game.board[index]) {
                cardButtons[index].hide()
            } else {
                cardButtons[index].backgroundColor = #colorLiteral(red: 1.0, green: 1.0, blue: 1.0, alpha: 1.0)
            }
        }
        deal3CardsButton.isUserInteractionEnabled = game.canDealMoreCards
        deal3CardsButton.backgroundColor = game.canDealMoreCards ? #colorLiteral(red: 1.0, green: 1.0, blue: 1.0, alpha: 1.0) : #colorLiteral(red: 0.8039215803, green: 0.8039215803, blue: 0.8039215803, alpha: 1)

    }

    let symbols = ["▲", "●", "■"]
    let colors = [#colorLiteral(red: 0.9254902005, green: 0.2352941185, blue: 0.1019607857, alpha: 1), #colorLiteral(red: 0.2392156869, green: 0.6745098233, blue: 0.9686274529, alpha: 1), #colorLiteral(red: 0.4666666687, green: 0.7647058964, blue: 0.2666666806, alpha: 1)]

    func getAttributedString (forCard card: Card) -> NSAttributedString {
        let symbol = symbols[card.symbol-1]
        let string = String(repeating: symbol, count: card.quantity)
        let color = colors[card.color-1]
        let shading = Shading(rawValue: card.shading)!
        var attributes: [NSAttributedStringKey: Any] = [
            .strokeWidth: -3,
            .strokeColor: color,
            .foregroundColor: color
        ]
        switch shading {
        case .fill:
            break
        case .open:
            attributes[.foregroundColor] = color.withAlphaComponent(0)
        case .striped:
            attributes[.foregroundColor] = color.withAlphaComponent(0.25)
        }

        return NSAttributedString(string: string, attributes: attributes)
    }

//    var circle = Symbol.circle

}

// ▲ ● ■

enum Shading: Int {
    case fill = 1, striped, open
}

extension UIButton {
    func hide() {
        self.backgroundColor = #colorLiteral(red: 1, green: 1, blue: 1, alpha: 0)
        self.setTitle(nil, for: UIControlState.normal)
        self.setAttributedTitle(nil, for: UIControlState.normal)
    }
}
\$\endgroup\$
  • \$\begingroup\$ Your doesMakeSet() checks if the 3 cards have any attribute in common (eg. all are green, or all are triangles). But that are not the game rules as described en.wikipedia.org/wiki/Set_(game). \$\endgroup\$ – Martin R Jan 10 '18 at 7:06
  • \$\begingroup\$ Good catch... Easy fix but certainly makes the game much more challenging \$\endgroup\$ – Eric Groom Jan 12 '18 at 1:13
1
\$\begingroup\$

In looking at your code, what stands out to me is not so much a problem with your model api but with your implementation (both in model and vc). Cleaning up the impl will lead to better understanding of your model.

Basically, your code is hard to understand. You have long methods without clear structure. The three I would focus on are: updateViewFromModel, cardChosen and canDealMoreCards.

For example, updateViewFromModel does all sorts of peeking into your model's data, dependent on how your model represents its data. Instead, you should be able to just ask the model if a card is selected, matched, etc.

Wouldn't you rather see something like this:

private func updateViewFromModel() {
    removeCompletedCards()
    processHighlights()
    updateScore()
    updateCardsRemaining()
}

FYI, this assignment is non-trivial, so if your code is working, that's already not bad!

\$\endgroup\$
  • \$\begingroup\$ That is a great suggestion, thank you. I definitely agree that it was a bit too complicated. Since posting this, I have expanded the app quite a bit and am now using a custom UIView subclass to draw the cards using Core Graphics instead of using unicode symbols on buttons. By doing this, I was able to remove some additional cruft from the view controller. However, my updateViewFromModel() was still a bit unwieldy and I ended up doing a similar thing to what you suggested in your answer. What is the general opinion on nested functions used just to namespace different sections of them? \$\endgroup\$ – Eric Groom Jan 17 '18 at 1:52
  • \$\begingroup\$ Well, I would phrase it a little differently. You're not arbitrarily breaking it into chunks. You're factoring it into (potentially) reusable pieces of code that have a single purpose. It's not that important whether you actually re-use it right away. If done well (requires judgement), the benefit in readability is invaluable. Always ask yourself, will this be easy to understand for the next programmer who touches this code (may be you, in 6 months)? If not, figure out what you can do to make it readable. \$\endgroup\$ – vortek Jan 17 '18 at 5:42

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.