A Start button handler

Question

I was told my code contains a lot of force unwrapping. I thought it's okay to do that if I am sure that the value operated won't be nil:

private var x: Int?
private var y: Int?

@IBAction func startButtonPressed(_ sender: UIButton) {
    
    guard let numberOfRooms = selectedRooms.text, !numberOfRooms.isEmpty else {
        return selectedRooms.placeholder = "type it, dude"
    } //if user typed something
    let rooms = Int(numberOfRooms)
                   
    x = Int(ceil(sqrt(Double(rooms!))))
    y = x //grab some values from user input
    maze = MazeGenerator(x!, y!) //generate a maze 
    hp = 2 * (x! * y!) //get hp value depending on user input
    hpLabel.text = "hp: \(hp)"
    
    currentX = getRandomX(x!) //get random value in 0...x
    currentY = getRandomY(y!)
    currentCell = maze?.maze[currentX!][currentY!] //game starts in a random part of the maze
                            
    for item in items.indices { //create and show some imageViews 
        items[item].location = (getRandomX(x!), getRandomX(y!)) 
        createItems(imageName: items[item].imageName)
        if items[item].location == (currentX!, currentY!) {
            reveal(index: item) 
        } else {
            hide(index: item) 
        }
    }
    refreshButtons() //refresh UI
    maze!.display() //print maze scheme in debug console
    }

Does it look fine? If not, what should be done?

Answer 1

Code review:

1. use a guard statements to transform optional values to values.

private var x: Int?
private var y: Int?

@IBAction func startButtonPressed(_ sender: UIButton) {
    
    guard let numberOfRooms = selectedRooms.text, !numberOfRooms.isEmpty else {
        return selectedRooms.placeholder = "type it, dude"
    } //if user typed something
    let rooms = Int(numberOfRooms)
                   
    x = Int(ceil(sqrt(Double(rooms!))))
    y = x //grab some values from user input
    
    guard let x = x, y = y else {
        return
    }
    guard let maze = MazeGenerator(x, y) else { //generate a maze
        return
    }
    hp = 2 * (x * y) //get hp value depending on user input
    hpLabel.text = "hp: \(hp)"
    
    guard let currentX = getRandomX(x), let currentY = getRandomY(y) else {
        return
    }
    currentCell = maze.maze[currentX][currentY] //game starts in a random part of the maze
                            
    for item in items.indices { //create and show some imageViews
        items[item].location = (getRandomX(x!), getRandomX(y!))
        createItems(imageName: items[item].imageName)
        if items[item].location == (currentX!, currentY!) {
            reveal(index: item)
        } else {
            hide(index: item)
        }
    }
    refreshButtons() //refresh UI
    maze.display() //print maze scheme in debug console
}

2. If possible, define a properties as non optional

private var x: Int = 0
private var y: Int = 0

@IBAction func startButtonPressed(_ sender: UIButton) {
    
    guard let numberOfRooms = selectedRooms.text, !numberOfRooms.isEmpty else {
        return selectedRooms.placeholder = "type it, dude"
    } //if user typed something
    let rooms = Int(numberOfRooms)
                   
    x = Int(ceil(sqrt(Double(rooms!))))
    y = x //grab some values from user input
    
    guard let maze = MazeGenerator(x, y) else { //generate a maze
        return
    }
    hp = 2 * (x * y) //get hp value depending on user input
    hpLabel.text = "hp: \(hp)"
    
    guard let currentX = getRandomX(x), let currentY = getRandomY(y) else {
        return
    }
    currentCell = maze.maze[currentX][currentY] //game starts in a random part of the maze
                            
    for item in items.indices { //create and show some imageViews
        items[item].location = (getRandomX(x!), getRandomX(y!))
        createItems(imageName: items[item].imageName)
        if items[item].location == (currentX!, currentY!) {
            reveal(index: item)
        } else {
            hide(index: item)
        }
    }
    refreshButtons() //refresh UI
    maze.display() //print maze scheme in debug console
}

3. @IBAction should call other methods instead executing a logic code. Example: One would trigger a game start method and an analytics event once a button tapped.

private var x: Int?
private var y: Int?

@IBAction func startButtonPressed(_ sender: UIButton) {
    gameStartButtonPressed()
    Analytics.logAction("startButtonPressed")
}

private func gameStartButtonPressed() {
    guard let numberOfRooms = selectedRooms.text, !numberOfRooms.isEmpty else {
        return selectedRooms.placeholder = "type it, dude"
    } //if user typed something
    let rooms = Int(numberOfRooms)
                   
    x = Int(ceil(sqrt(Double(rooms!))))
    y = x //grab some values from user input
    
    guard let x = x, y = y else {
        return
    }
    guard let maze = MazeGenerator(x, y) else { //generate a maze
        return
    }
    hp = 2 * (x * y) //get hp value depending on user input
    hpLabel.text = "hp: \(hp)"
    
    guard let currentX = getRandomX(x), let currentY = getRandomY(y) else {
        return
    }
    currentCell = maze.maze[currentX][currentY] //game starts in a random part of the maze
                            
    for item in items.indices { //create and show some imageViews
        items[item].location = (getRandomX(x!), getRandomX(y!))
        createItems(imageName: items[item].imageName)
        if items[item].location == (currentX!, currentY!) {
            reveal(index: item)
        } else {
            hide(index: item)
        }
    }
    refreshButtons() //refresh UI
    maze.display() //print maze scheme in debug console
}

4. Separate a model from a controller

struct Model {
    var x: Int
    var y: Int
}

private var position: Model = .init(x: 0, y: 0)

// instead of:

private var x: Int?
private var y: Int?

Additional insights:

Force unwrapping is useful in following cases (this list is not exhaustive):

• failable initializers
• functions or methods returning optional type instead throwing an error
• IBOutlet attribute

Example of a failable initializers:

URL type has a failable initialiser init?(string:) which return optional URL object. In case of a string "https://www.google.pl" one can be sure that the string results in correct url object and not nil. In that case force unwrap is the way to express this having confidence that instead obtaining optional URL, one expects an URL object.

//failable initializer creates an optional URL object
let url: URL? = URL(string: "https://www.google.pl")

//force unwrapping leads to a non optional URL object
let url: URL = URL(string: "https://www.google.pl")!

Answer 2

Yes, force unwrapping is ok if you're 100% sure the value won't be nil, but this should only be used when, for code structure reasons, you can't do otherwise. The thing is, most of the times, you can do otherwise, so using force-unwrapping should rarely happen. In your case, I think most of it can be refactored.

For example: why are your variables x and y declared as Optionals in the first place? Do they have to be?

If they are only used in this method, they should not be declared as Optional, and they should be declared inside the method, not outside.

If they are used elsewhere, they should be declared as Optional only if you know they will be nil at some point (which means that they should not be force unwrapped when used, but instead properly validated with error handling).

If they are used elsewhere and won't be nil when used, but you declared them as Optional for structural reasons, then you could use an implicitly unwrapped Optional instead.

private var x: Int!
private var y: Int!

This way you won't have to force unwrap them each time you use them.

Also, if this is acceptable in your context, you could make them non-optional and have an initial value.

private var x: Int = 0
private var y: Int = 0

But let's say that you have a good reason to declare them as Optionals. In this case, you still can improve your code by safely unwrapping the variables before using them.

guard let x = Int(ceil(sqrt(Double(rooms!)))) else {
    // show some message
    return
}

That way, if the user types a letter, space or non-numeric character, instead of a number, the app won't crash.

Maybe you're making sure elsewhere that the user can only input a number? In this case, force unwrap x before using it:

x = Int(ceil(sqrt(Double(rooms!))))!

Now, actually, the issue might only happen if numberOfRooms is not a number, so this unwrapping should happen earlier.

guard let numberOfRooms = selectedRooms.text, !numberOfRooms.isEmpty, let rooms = Int(numberOfRooms) else {
    // error message 'please input a number'
    return
}

Same idea about currentX and currentY: why are these declared as Optionals? Can the getRandom method return nil? Maybe that can be changed?

Same for maze: if MazeGenerator can return nil, the shouldn't you handle possible errors instead of force unwrapping? And if it won't ever return nil, then why does it return an Optional instead of a normal value? (or maybe it doesn't return an Optional and the issue is that maze was declared as an Optional but shouldn't have been).

---

These are the issues you should address.

While force unwrapping is indeed not bad if you can guarantee the value won't be nil when you use it, most of the times it's better to make sure earlier in the code flow that the value won't be nil, which will allow you to not having to use Optionals.

In short: validate the values before use, abort and show an error message if the content is incorrect at the validation step, and then proceed to use the values without having to use Optionals and unwrapping.