iOS Set game implementation

Question

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

• Martin R: 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).
• Eric Groom: Good catch... Easy fix but certainly makes the game much more challenging

Answer 1

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!

• Eric Groom: 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?
• vortek: 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.