HiveBrain v1.2.0
Get Started
← Back to all entries
patternswiftMinor

Calculator implementation

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
implementationcalculatorstackoverflow

Problem

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 = disp

Solution

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

Code Snippets

var displayValue : Double {
    get {
        return (display.text as NSString!).doubleValue
    }
    set {
    }
}
var displayValue : Double {
    get {
        return (display.text as NSString!).doubleValue
    }
}
var displayValue : Double {
    get {
        return (display.text as NSString!).doubleValue
    }
    set {
        display.text = String(format: "%f", newValue)
    }
}
enum MathematicalOperation: String {
    case Addition = "+"
    case Subtraction = "-"
    case Multiplication = "x"
    case Division = "÷"
}
var currentOperation : MathematicalOperation?
var nextOperation : MathematicalOperation?

Context

StackExchange Code Review Q#90143, answer score: 4

Revisions (0)

No revisions yet.