Finding a word that represents an image

Question

This is an app wherein you have to find the word that represents the image. It's the same concept as the course app but I did this one myself.

How can I improve this code (put more commentary, put more space, you can add that, etc.)? I finished the second part of app development with Swift. Please explain why I should do the modification. Commentary are in French but I can translate them.

Game Struct

import Foundation
import UIKit
struct Game
{
    var correctWord: String
    var image: UIImage
    var guessedLetter: [Character]
    // création du bon mot par le joueur
    var formatedWord: String
    {
        var formateWord = ""
        let word = correctWord.lowercased()

        for letter in word
        {
            if guessedLetter.contains(letter)
            {
                formateWord += "\(letter)"
            }
            else
            {
                formateWord += "_"
            }
        }

        return formateWord
    }
    // Quand le joueur
    mutating func playerGuessed(_ letter: Character)
    {
        guessedLetter.append(letter)
    }
}

View Controller

import UIKit
//liste de mot et image
var imageAndWord = ["Maison": #imageLiteral(resourceName: "image 1"),"Bateau": #imageLiteral(resourceName: "image2") ,"Train": #imageLiteral(resourceName: "image 3"),"Vase": #imageLiteral(resourceName: "image 4")]
//récupération des clées
var theKey = Array(imageAndWord.keys)
// Nombre de clé
let totalKey = theKey.count + 1
//récupération des valeurs
var theValue = Array(imageAndWord.values)
// comptage d'image trouvé
var imageFindNumber = 0
class ViewController: UIViewController {
    // initialisation des vues
    @IBOutlet var lettersButtons: [UIButton]!
    @IBOutlet weak var imageView: UIImageView!
    @IBOutlet weak var wordLabel: UILabel!
    @IBOutlet weak var scoreLabel: UILabel!
    //Quand une lettre est pressée
    @IBAction func lettersButtonsPressed(_ sender: UIButton)
    {
        sender.isEnabled = false
        let letter = sender.title(for: .normal)!
        let lowCasedLetter = Character(letter.lowercased())
        currentGame.playerGuessed(lowCasedLetter)
        if currentGame.correctWord == currentGame.formatedWord
        {
            newRound()

        }
        updateUI()
    }
    // Actualisation de l'interface
    func updateUI()
    {
        let word = currentGame.formatedWord.map{String($0)}
        let spacingWord = word.joined(separator: " ")
        wordLabel.text = spacingWord
        imageView.image = currentGame.image
        scoreLabel.text = "Image: \(imageFindNumber)/\(totalKey)"

    }
    var currentGame: Game!
    // Initialisation de l'image et du mot
    func newRound()
    {
        if !theKey.isEmpty && !theValue.isEmpty
        {
        enableOrNotButton(true)
        let word = theKey.removeFirst().lowercased()
        let value = theValue.removeFirst()
        currentGame = Game(correctWord:word, image:value, guessedLetter: [])
        imageFindNumber += 1
        updateUI()
        }
        else
        {
            enableOrNotButton(false)
        }
    }
    //Activation ou désactivation de tout les bouttons
    func enableOrNotButton(_ enable: Bool)
    {
            for button in lettersButtons
            {
                button.isEnabled = enable
            }
    }
    override func viewDidLoad() {
        super.viewDidLoad()
        newRound()
        print(currentGame.correctWord)
        // Do any additional setup after loading the view, typically from a nib.
    }

    override func didReceiveMemoryWarning() {
        super.didReceiveMemoryWarning()
        // Dispose of any resources that can be recreated.
    }


}

Answer 1

I would begin with the imports. When importing UIKit you will not need to import Foundation because the contents of UIKit will also include Foundation.

Now to recommend a few things with regards to the Game struct. Now the way the struct is instantiated means that one image is equivalent to a "Game," so you would be able to set the correctWord, and image to constants with let.

Secondly, we can also provide a more descriptive names for the variables to eliminate any confusion later on down the line, like using guessedLetters instead of guessedLetter (plural indicates that this is likely an array), correctAnswer instead of correctWord, and guessedWord instead of formatedWord as these are more clear and descriptive of their purposes.

import UIKit

struct Game
{
    let correctAnswer: String
    let image: UIImage
    var guessedLetters: [Character]
    // création du bon mot par le joueur
    var guessedAnswer: String
    {
        var guessedAnswer = ""
        let answer = correctAnswer.lowercased()

        for letter in answer
        {
            if guessedLetters.contains(letter)
            {
                guessedAnswer += "\(letter)"
            }
            else
            {
                guessedAnswer += "_"
            }
        }

        return guessedAnswer
    }
    // Quand le joueur
    mutating func playerGuessed(_ letter: Character)
    {
        guessedLetters.append(letter)
    }
}

Now with regards to the View Controller. I would begin by dealing with the variables at the top of the controller. These should either be included in the controller or refactored into their own file. For simplicity sake, I'll just move these to be part of the ViewController.

Next we would want to rename some of the variables to make their purposes more clear. This will help with understanding of what everything does in a way that is more identifiable than comments. I've opted to use names like imageDictionary and currentGameIndex over imageAndWord and imageFindNumber, for example.

Next, there are a couple values that are unnecessary. Particularly theKey, theValue, and totalKey. These are not needed because we already have a mutable dictionary that we can perform actions against, so we do not need to create extra versions of these values. When we are setting the new game parameters, I've updated those references to use the following logic, which will do the same thing without needing additional arrays.

imageDictionary.removeValue(forKey: currentGame.correctAnswer)
let word = imageDictionary.first?.key.lowercased()
let value = imageDictionary.first?.value

I've introduced a guard statement in the newRound method. This statement is useful for when you have a dependency that you need to continue executing. In this case, you need to have an image available in order to proceed, so the guard case will verify if there is one, and then continue through or terminate early. This is not necessary, but in cases where there are dependencies that are required, it can be useful and easier to read than nested if statements.

Another change I would recommend is renaming the enableOrNotButton: method. In my example, I've updated it to setButtonStatus enabled: as it helps you understand at a glance what the intent is. setButtonStatus(enabled:true) shows you that you are intending that the button will be updated to enable = true.

Finally I've removed the redundant comments, as those don't really help much of anything. If the method is named appropriately, as is the case for updateUI(), then you don't really need a comment that says "updateUI() will update the UI". Comments are more helpful when they explain something that isn't understood. A good comment should be able to answer a question or provide information that isn't immediately obvious.

import UIKit

class ViewController: UIViewController {

    var imageDictionary = ["Maison": #imageLiteral(resourceName: "image 1"),"Bateau": #imageLiteral(resourceName: "image2") ,"Train": #imageLiteral(resourceName: "image 3"),"Vase": #imageLiteral(resourceName: "image 4")]
    var currentGameIndex = 0
    var currentGame: Game!

    @IBOutlet var letterButtons: [UIButton]!
    @IBOutlet weak var imageView: UIImageView!
    @IBOutlet weak var wordLabel: UILabel!
    @IBOutlet weak var scoreLabel: UILabel!

    @IBAction func letterButtonsPressed(_ sender: UIButton)
    {
        // Disable the button so the user cannot select the letter again.
        sender.isEnabled = false
        let guessedLetter = sender.title(for: .normal)!
        let lowercasedLetter = Character(guessedLetter.lowercased())
        currentGame.playerGuessed(lowercasedLetter)

        if currentGame.correctAnswer == currentGame.guessedAnswer
        {
            beginNewRound()
        }

        updateUI()
    }

    func updateUI()
    {
        let word = currentGame.guessedAnswer.map{String($0)}
        let spacingWord = word.joined(separator: " ")
        wordLabel.text = spacingWord
        imageView.image = currentGame.image
        scoreLabel.text = "Image: \(currentGameIndex)/\(totalKeys)"

    }

    func beginNewRound()
    {
        guard imageDictionary.isEmpty else {
            setButtonStatus(enabled:false)
            return
        }

        setButtonStatus(enabled:true)
        let word = imageDictionary.first?.key.lowercased()
        let value = imageDictionary.first?.value
        currentGame = Game(correctWord:word, image:value, guessedLetter: [])
        currentGameIndex += 1
        imageDictionary.removeValue(forKey: currentGame.correctAnswer)
        updateUI()
    }

    func setButtonStatus(enabled status: Bool)
    {
            for button in lettersButtons
            {
                button.isEnabled = status
            }
    }

    override func viewDidLoad() {
        super.viewDidLoad()
        beginNewRound()
        print(currentGame.correctAnswer)
        // Do any additional setup after loading the view, typically from a nib.
    }

    override func didReceiveMemoryWarning() {
        super.didReceiveMemoryWarning()
        // Dispose of any resources that can be recreated.
    }


}

Answer 2

Some points in addition to jlowe's excellent and comprehensive answer (and I'll mainly use the already suggested improved property names):

We care only about distinct guessed letters (even if the user guesses the same letter twice), therefore a set is a more appropriate data structure than an array:

var guessedLetters: Set<Character>

mutating func playerGuessed(_ letter: Character) {
    guessedLetters.insert(letter)
}

Instead of the string interpolation in

guessedAnswer += "\(letter)"

one can append a character directly:

guessedAnswer.append(letter)

Using the conditional operator ?: the computed property can be simplified to

var guessedAnswer: String {
    var guessedAnswer = ""
    let answer = correctAnswer.lowercased()
    for letter in answer {
        guessedAnswer.append(guessedLetters.contains(letter) ? letter : "_")
    }
    return guessedAnswer
}

But what that actually does is to map each character of the correct answer to a new character, and that's what map() is for:

var guessedAnswer: String {
    return String(correctAnswer.lowercased()
        .map { guessedLetters.contains($0) ? $0 : "_" })
}

Finally, in

currentGame = Game(correctWord:word, image:value, guessedLetter: [])

the default memberwise initializer is used. But there should be no need to pass an empty list as the last argument. Better set an initial value for that property, and define an init method:

struct Game {
    let correctAnswer: String
    let image: UIImage
    var guessedLetters = Set<Character>()

    init(answer: String, image: UIImage) {
        self.correctAnswer = answer
        self.image = image
    }

    // ...
}

and create new instances with

currentGame = Game(answer: word, image: image)