4
\$\begingroup\$

I just started learning the basics to Swift 3 about 2 days ago. Since then, I've decided to make a simple calculator that can only do one operation at a time.

I was wondering how efficient this was and what I could possibly do to make it more efficient. Any type of criticism would be helpful too

import UIKit

class ViewController: UIViewController {

    @IBOutlet var buttonResponseLabel: UILabel!
    var userIsCurrentlyTyping: Bool = false
    var storedValue: Int = 0;
    var currentOperation = 0
    var currentNumberInLabel = ""

    @IBAction func buttonAppend(_ sender: UIButton)
    {
        let buttonTitle = sender.currentTitle!


        switch buttonTitle
        {
            case "+":
            currentOperation = 1
            storedValue = Int(currentNumberInLabel)!
            buttonResponseLabel.text = ""
            case "-":
            currentOperation = 2
            storedValue = Int(currentNumberInLabel)!
            buttonResponseLabel.text = ""
            case "*":
            currentOperation = 3
            storedValue = Int(currentNumberInLabel)!
            buttonResponseLabel.text = ""
            case "/":
            currentOperation = 4
            storedValue = Int(currentNumberInLabel)!
            buttonResponseLabel.text = ""
            default:


            if buttonTitle == "="
            {
                var finalValue = 0
                print(currentNumberInLabel)
                let secondStoredValue = Int(currentNumberInLabel)!
                if currentOperation == 1
                {
                    finalValue = storedValue + secondStoredValue
                    buttonResponseLabel.text = ""
                    buttonResponseLabel.text = String(finalValue)
                }
                else if currentOperation == 2
                {
                    finalValue = storedValue - secondStoredValue
                    buttonResponseLabel.text = ""
                    buttonResponseLabel.text = String(finalValue)
                }
                else if currentOperation == 3
                {
                    finalValue = storedValue * secondStoredValue
                    buttonResponseLabel.text = ""
                    buttonResponseLabel.text = String(finalValue)
                }
                else if currentOperation == 4
                {
                    finalValue = storedValue / secondStoredValue
                    buttonResponseLabel.text = ""
                    buttonResponseLabel.text = String(finalValue)
                }
            }
            else
            {
                if userIsCurrentlyTyping
                {
                    currentNumberInLabel = buttonResponseLabel.text! + buttonTitle
                    buttonResponseLabel.text = currentNumberInLabel
                }
                else
                {
                    userIsCurrentlyTyping = true
                    buttonResponseLabel.text = ""
                    buttonResponseLabel.text = buttonTitle
                }

            }

            print("\(currentOperation) \(buttonTitle)")
        }

    }

}
\$\endgroup\$
  • 1
    \$\begingroup\$ Efficiency shouldn't be your focus at this point. Aim for correctness first (write unit tests!), then refactor to remove duplication; then consider continued refactoring to improve the design and readability/maintainability. Only then should you worry about efficiency; even then, you only need to improve efficiency if the existing code is causing an actual performance issue. \$\endgroup\$ – John Deters Dec 8 '16 at 16:09
3
\$\begingroup\$

Try not to repeat yourself and refactor common code to functions. Also you might want to create types (classes, structs, enums) to make things more expressive. Quick example with not so nice new data type (untested):

import UIKit

class ViewController: UIViewController {

    struct Operator {
        var stringRepresentation: String
        var `operator`: (Int, Int) -> Int //quotes required  as operator is a keyword
    }

    var operators = ["+":Operator(stringRepresentation:"+", operator:+),
                 "-":Operator(stringRepresentation:"-", operator:-),
                 "*":Operator(stringRepresentation:"*", operator:*),
                 "/":Operator(stringRepresentation:"/", operator:/)]
    @IBOutlet var buttonResponseLabel: UILabel!
    var userIsCurrentlyTyping: Bool = false
    var storedValue: Int = 0;
    var currentOperation: Operator?
    var currentNumberInLabel = ""
    var buttonTitle = ""

    @IBAction func buttonAppend(_ sender: UIButton) {
        buttonTitle = sender.currentTitle!

        if let chosenOperator = operators[buttonTitle] {
            currentOperation = chosenOperator
            storedValue = Int(currentNumberInLabel)!
            buttonResponseLabel.text = ""
        } else if buttonTitle == "=" {
            showResult()
        } else if userIsCurrentlyTyping {
            currentNumberInLabel = buttonResponseLabel.text! + buttonTitle
            buttonResponseLabel.text = currentNumberInLabel
        } else {
            userIsCurrentlyTyping = true
            buttonResponseLabel.text = buttonTitle
        }

        print("\(currentOperation) \(buttonTitle)")
    }

    func showResult() {
        print(currentNumberInLabel)
        guard let currentOperator = currentOperation else {
            print("This should never happen. Fail silently")
            return
        }
        operate(withOperator: currentOperator.operator)
    }

    func operate(withOperator op: (Int, Int) -> Int){
        let secondStoredValue = Int(currentNumberInLabel)!
        let finalValue = op(storedValue, secondStoredValue)
        buttonResponseLabel.text = String(finalValue)
    }
}

A very good walk-through Swift by implementing a calculator can be found here btw. It's from late 2015, so no Swift3. Maybe you find an update.

\$\endgroup\$
  • \$\begingroup\$ Any way to explain what is going on in this part of the code? var operators = ["+":Operator(stringRepresentation:"+", theOperator:+), "-":Operator(stringRepresentation:"-", theOperator:-), "*":Operator(stringRepresentation:"*", theOperator:*), "/":Operator(stringRepresentation:"/", theOperator:/)] \$\endgroup\$ – bsoleimany Dec 7 '16 at 23:20
  • \$\begingroup\$ This is instantiating a Dictionary or type [String:Operator] with an entry for each possible operator. It is a short for for: var operators = [String:Operator]() operators["+"] = Operator(stringRepresentation:"+", theOperator:+) operators["-"] = Operator(stringRepresentation:"-", theOperator:-) ... \$\endgroup\$ – shallowThought Dec 8 '16 at 13:28
1
\$\begingroup\$

Something that I really enjoy doing in Swift is making use of of enumerations.

You could create a CalculatorOperation enum that extracts some of your code into an type like this:

enum CalculatorOperation {

    case add
    case subtract
    case multiply
    case divide

    init?(from buttonTitle: String) {
        switch buttonTitle {
        case "+": self = .add
        case "-": self = .subtract
        case "*": self = .multiply
        case "/": self = .divide
        default: return nil
        }
    }

    func apply(to left: Int, and right: Int) -> Int {

        switch self {
        case .add:
            return left + right

        case .subtract:
            return left - right

        case .multiply:
            return left * right

        case .divide:
            return left / right
        }
    }
}

Then, your view controller becomes a lot more manageable.

I'd also separate out your @IBActions so that you have one for when people tap on an operation button, and another action specifically for dealing with when people tap the '=' button.

class ViewController: UIViewController {

    @IBOutlet var inputLabel: UILabel!

    var firstNumber: Int = 0
    var currentOperation: CalculatorOperation? = nil

    // connect 1,2,3,4,5,6,7,8,9,0 buttons to this action
    @IBAction func updateNumber(_ numberButton: UIButton) {

        // update the input label with whatever number has been pressed
        let numberTitle = numberButton.currentTitle!
        inputLabel.text = inputLabel.text! + numberTitle
    }

    // connect '+', '-', '*', '/' buttons to this action
    @IBAction func updateOperation(_ operationButton: UIButton) {

        // update the operation based on the key pressed
        let operationTitle = operationButton.currentTitle!
        currentOperation = CalculatorOperation(from: operationTitle)

        // save the number shown in the input as the first number
        let currentinput = inputLabel.text!
        firstNumber = Int(currentinput)!

        // clear out the input label so it's ready for new number to be entered
        inputLabel.text = ""
    }

    // connect '=' button to this action
    @IBAction func performCalculation(_ sender: UIButton) {

        // get the second number from whatever is currently in the input label
        let currentinput = inputLabel.text!
        let secondNumber = Int(currentinput)!

        let finalValue = currentOperation!.apply(to: firstNumber, and: secondNumber)

        // update input label with the calculated value
        inputLabel.text = String(finalValue)
    }
}

Something else I've found really helpful lately is to start in a playground, and make sure that my logic is working before jumping into a an app.

The playgrounds are always re-compiling in the background as you type, so it's a great way to get early feedback on logic errors and spot bugs.

Here's an example where I noticed that the division isn't worked as expected because we're using Int (so should probably cast to a Double)

enter image description here

\$\endgroup\$
  • \$\begingroup\$ You should make this enum have a String rawValue, so it gets rid of the need of manually making an initializer that switches on strings. \$\endgroup\$ – Alexander - Reinstate Monica Jan 23 '17 at 21:51

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.