Calculator implementation

Question

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 {
 }
}
}

Answer 1

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)"
 }
}

• Cirrocumulus: 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)?
• nhgrif: @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?
• Cirrocumulus: 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.