7
\$\begingroup\$

With some background and experience in PHP & Python, I'm trying to learn Swift by myself (web, videos, Ray Wenderlich books). I've read that a good first project for beginners is to try and write a basic calculator, so I've tried that. I'm wondering if there's a way to make my code leaner and more elegant (I've spent quite some time just getting the operator & equals keys to behave as expected). There's still no support for handling division by zero or display overflowing, the "C" and "AC" methods should be merged, but for now it does what it's supposed to do.

All digit keys are connected to buttonDigit, all operators to operation and the rest is obvious (I hope). Pardon the indentation right after the class declaration.

import UIKit

class ViewController: UIViewController {

@IBOutlet weak var display: UILabel!
var isTyping : Bool = false
var currentResult : Double = 0
var currentOperation : String? = nil
var nextOperation : String? = nil
var operand : Double = 0

// Handle digits

@IBAction func buttonDigit(sender: UIButton) {

    let digit = sender.currentTitle!
    if isTyping {
        display.text = display.text! + digit
    } else {
        display.text = digit
        isTyping = true
    }

}

// Handle operations keys

@IBAction func operation(sender: UIButton) {

    operand = displayValue
    nextOperation = sender.currentTitle

    if isTyping {
        equals()
    }

    currentOperation = nextOperation
    currentResult = displayValue
    isTyping = false
}

// Handle "equals" key

@IBAction func equals() {

    if (isTyping) {
        operand = displayValue
    }

    if (currentOperation != nil) {
        switch currentOperation! {
        case "+": display.text = "\(currentResult + operand)"
        case "-": display.text = "\(currentResult - operand)"
        case "×": display.text = "\(currentResult * operand)"
        case "÷": display.text = "\(currentResult / operand)"
        default: break
        }
    }

    currentResult = displayValue
    isTyping = false
}

// Handle AC key

@IBAction func allClear() {
    currentResult = 0
    display.text = "0"
    nextOperation = nil
    currentOperation = nil
    isTyping = false
}

// Handle C key

@IBAction func clear() {
    display.text = "0"
    isTyping = false
}

// Computed double from display label string

var displayValue : Double {
    get {
        return (display.text as NSString!).doubleValue
    }
    set {
    }
}
}
\$\endgroup\$
4
\$\begingroup\$
var displayValue : Double {
    get {
        return (display.text as NSString!).doubleValue
    }
    set {
    }
}

This empty setter is very disingenuous.

If we don't want to let the user set the display's text property by passing a double, then we should make this property readonly by eliminating the set:

var displayValue : Double {
    get {
        return (display.text as NSString!).doubleValue
    }
}

But I actually don't necessarily see a reason why this can't be readwrite:

var displayValue : Double {
    get {
        return (display.text as NSString!).doubleValue
    }
    set {
        display.text = String(format: "%f", newValue)
    }
}

Of course, we can use our format string to specify number of decimal places to show and such.

The biggest thing is, we need to do something with the empty set for our computed property. If we don't want an end-user setting the value this way, then don't let them and don't let them think they can. By removing the set method as I've done in the first example, trying to set the property will result in a compiler error.


There's another area we want to start using the compiler to our advantage--with our operations. You're using a simple String type to track what sort of math we should be doing. Swift is all about explicit types, so let's make an enum to handle our operations:

enum MathematicalOperation: String {
    case Addition = "+"
    case Subtraction = "-"
    case Multiplication = "x"
    case Division = "÷"
}

Now let's just our currentOperation and nextOperation to have the MathematicalOperation type:

var currentOperation : MathematicalOperation?
var nextOperation : MathematicalOperation?

We can still make use of the button's title text if we really want:

self.nextOperation = MathematicalOperation(rawValue:sender.currentTitle)

And now, let's fix up that switch:

if (currentOperation != nil) {
    switch currentOperation! {
    case "+": display.text = "\(currentResult + operand)"
    case "-": display.text = "\(currentResult - operand)"
    case "×": display.text = "\(currentResult * operand)"
    case "÷": display.text = "\(currentResult / operand)"
    default: break
    }
}

Let's get rid of the unnecessary ! operation (which when abused leads to some bad code--here is fine, but let's not get in the habbit), and let's not rely on string comparisons for the switch:

if let op = currentOperation {
    switch op {
        case .Addition: display.text = "\(currentResult + operand)"
        case .Subtraction: display.text = "\(currentResult - operand)"
        case .Multiplication: display.text = "\(currentResult * operand)"
        case .Division: display.text = "\(currentResult / operand)"
    }
}
\$\endgroup\$
  • \$\begingroup\$ Thank you. I had left the empty set method thinking I would come back to it later; I didn't have any use for a setter and didn't know I could simply drop it. I was rather interested in knowing if my implementation was convoluted or if I had to have so much code for a simple app. I assumed a calculator had been programmed a zillion times and that a "best practice" was already common knowledge among programmers. But I'm curious: why would an end user see any of this? If I don't code in a team, why would anyone care about an empty setter (except for myself, for elegance or safety reasons)? \$\endgroup\$ – Cirrocumulus May 10 '15 at 21:15
  • \$\begingroup\$ @Cirrocumulus I just added some more to this answer you should double-check. If you're not programming in a team, no one else would see this most likely, but that doesn't mean we should be lax in our standards. Plus, what happens if you put this application down and come back 18 months later having forgotten basically everything about the app? \$\endgroup\$ – nhgrif May 10 '15 at 21:21
  • \$\begingroup\$ Thanks again. This is indeed much better. I've sorted out a few bugs and added decimals & memory features, in case anyone's interested in looking at and giving feedback on new code I can post a more complete version. \$\endgroup\$ – Cirrocumulus May 12 '15 at 8:34

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.