How to shorten this code to prevent duplication of code?

Question

I had this question for quite a bit long. I am trying to visualize the 3 doors problem, just for fun and practice with Swift. So I have:

3 doors, and therefore 3 different IBActions & 3 functions for all doors. These function are all exactly the same, however only the number of the doors are different in each code. So I was wondering, can I shorten this code?:

func openSecondChoice(whatDoorIsClickedOn: Int)
    {
        if whatDoorIsClickedOn == 1
        {
            if whatDoorIsClickedOn == doorWithNumber
            {
                UIButtonDoor1.setBackgroundImage( UIImage (named: "doorWithMoney"), for: UIControlState.normal)
            }
            else
            {
                UIButtonDoor1.setBackgroundImage( UIImage (named: "doorWithGoat"), for: UIControlState.normal)
            }
        }
        if whatDoorIsClickedOn == 2
        {
            if whatDoorIsClickedOn == doorWithNumber
            {
                UIButtonDoor2.setBackgroundImage( UIImage (named: "doorWithMoney"), for: UIControlState.normal)
            }
            else
            {
                UIButtonDoor2.setBackgroundImage( UIImage (named: "doorWithGoat"), for: UIControlState.normal)
            }
        }
        if whatDoorIsClickedOn == 3
        {
            if whatDoorIsClickedOn == doorWithNumber
            {
                UIButtonDoor3.setBackgroundImage( UIImage (named: "doorWithMoney"), for: UIControlState.normal)
            }
            else
            {
                UIButtonDoor3.setBackgroundImage( UIImage (named: "doorWithGoat"), for: UIControlState.normal)
            }
        }
    }

Yuk! This code is so ugly! If the user presses on door1 for example, I'm calling the function "openSecondChoise(whatDoorIsClickedOn: 1)". Is there a way to shorten this? Thank you! I do not use classes here, should I use them?

You should post this on http://codereview.stackexchange.com instead of here. — rmaddy, Dec 21 '16 at 19:31

Emil Laine · Accepted Answer · 2016-12-21T18:58:10.147

5

Usually when you start suffixing your variable names with 1, 2, 3, etc., it's time to use an array instead. This is what arrays are for.

With an array uiButtonDoors that contains your UIButtonDoor1...UIButtonDoor3, your function could look like this:

func openSecondChoice(whatDoorIsClickedOn: Int) {
    let imageName = whatDoorIsClickedOn == doorWithNumber ? "doorWithMoney" : "doorWithGoat"
    uiButtonDoors[whatDoorIsClickedOn - 1].setBackgroundImage(UIImage(named: imageName), for: UIControlState.normal)
}

edited Dec 21 '16 at 18:58

answered Dec 21 '16 at 18:47

Emil Laine

41,598
9
101
157

This will crash for `whatDoorIsClickedOn == 3` – CodingMeSwiftly Dec 21 '16 at 18:53
Np. I think your solution - while shorter than mine - is a tad more inconvenient, because OP would need to change the structure of her code outside the method itself... I suppose it's really just a matter of taste here. However, I totally agree on using an array of buttons for cleaner code :) – CodingMeSwiftly Dec 21 '16 at 18:57
Its okay, I am used to change my whole code hehe :) Question for you guys, I already asked it also to the person who gave the last answer, so maybe he already answered it... but you are using this: "let imageName = whatDoorIsClickedOn == doorWithNumber ? "doorWithMoney" : "doorWithGoat" What is this exactly? What does it do? Why do you use "==" instead of "="? I thought you only use "==" when referring, so its more like a "check" function instead of actually changing something. I hope you understand this! Thank you! – Petravd1994 Dec 21 '16 at 23:04

CodingMeSwiftly · Answer 2 · 2016-12-21T19:01:06.217

3

func openSecondChoice(whatDoorIsClickedOn: Int) {
  let imageName = whatDoorIsClickedOn == doorWithNumber ? "doorWithMoney" : "doorWithGoat"
  let image = UIImage(named: imageName)

  let button: UIButton

  switch whatDoorIsClickedOn {
  case 1:
    button = UIButtonDoor1
  case 2:
    button = UIButtonDoor2
  case 3:
    button = UIButtonDoor3
  default:
    fatalError("Cannot be. Switch must be exhaustive, that's why we need to use 'default' for a switch on Int.")
  }

  button.setBackgroundImage(image, for: .normal)
}

For a shorter version check @tuple_cat s answer.

edited Dec 21 '16 at 19:01

answered Dec 21 '16 at 18:52

CodingMeSwiftly

3,231
21
33

I used a couple of ternary conditional operators instead of a switch, but the same effect. @tuple_cat has a much better answer. – Jeffery Thomas Dec 21 '16 at 19:12
1

Or, instead of the whole switch: `let button = [UIButtonDoor1, UIButtonDoor2, UIButtonDoor3][whatDoorIsClickedOn - 1]` – Nikolai Ruhe Dec 21 '16 at 19:15
Thank you for your time. However, you are saying that tuple_cat has a better answer, so I will look onto that. But a question for you, why use switch statement over an if statement? I searched it up, I got this answer: http://stackoverflow.com/questions/1028437/why-switch-case-and-not-if-else-if Is it still valid or outdated? – Petravd1994 Dec 21 '16 at 23:00
Sure it's still valid. `switch` is just another tool in your toolbox besides `if-else`. There are many advantages but you should always ponder depending on the use case. However, personally I find myself using `switch` a lot more often since I started coding in Swift. It just feels more natural to the language, especially when you start using pattern matching all over the place :) – CodingMeSwiftly Dec 21 '16 at 23:14

par · Answer 3 · 2016-12-21T19:15:08.137

2

Another approach, for fun.

import UIKit

class DoorGame {
    func setupButtons() {
        let buttons = [UIButton(), UIButton(), UIButton()]

        for (index, button) in buttons.enumerated() {
            button.addTarget(self, action: #selector(buttonTapped(_:)), for: .touchUpInside)
            button.setTitle("\(index)", for: .normal)
        }

        let winningIndex = Int(arc4random_uniform(UInt32(buttons.count)))

        buttons[winningIndex].tag = 1
    }

    @objc func buttonTapped(_ sender: UIButton) {
        let imageName = sender.tag == 1 ? "doorWithMoney" : "doorWithGoat"
        let image = UIImage(named: imageName)

        sender.setBackgroundImage(image, for: .normal)
    }
}

edited Dec 21 '16 at 19:15

answered Dec 21 '16 at 19:06

par

17,361
4
65
80

Wow! This is different than the answers above. However thank you for your time. I do not understand all of this, for example: Why use a class here over a function? Why do you use "@objc" before the "fun buttonTapped"? And last: what is " let imageName = sender.tag == 1 ? "doorWithMoney" : "doorWithGoat"" How do you call it? Was does it do? Is it a condition and ":" separates the correct answer from the wrong answer if the condition is met (condition is everything before "?")? Thank you! – Petravd1994 Dec 21 '16 at 22:58
The theme of this answer is to use the `tag` property to embed some data on a button (`tag == 1` means it's the winner), and then to use the touch-up action on the button to alter the background image. It's in a class because encapsulation is a hallmark of good program design. `@objc` is an advanced topic so I'll recommend you read about it later. Normally this code would be in a view controller and you wouldn't need `@objc`, I just wanted this to compile in a Swift playground which is why I included it. `?:` is the conditional operator. It's a shorthand `if/else` statement. Good luck! – par Dec 22 '16 at 01:39

How to shorten this code to prevent duplication of code?

3 Answers3