Clear function for Stanford iOS 8 Calculator application

Question

I am following the Stanford iOS 8 lectures and I believe I have successfully added the "Clear" functionality to the application, but I don't know if I am doing it properly.

Controller

import UIKit
class ViewController: UIViewController
{
 @IBOutlet weak var display: UILabel!
 var userIsInTheMiddleOfTypingANumber = false
 var brain = CalculatorBrain()
 var displayValue: Double
 {
 get
 {
 return NSNumberFormatter().numberFromString(display.text!)!.doubleValue
 }
 set
 {
 display.text = "\(newValue)"
 userIsInTheMiddleOfTypingANumber = false
 }
 }
 @IBAction func appendDigit(sender: UIButton)
 {
 let digit = sender.currentTitle!
 if userIsInTheMiddleOfTypingANumber
 {
 display.text = display.text! + digit
 }
 else
 {
 display.text = digit
 userIsInTheMiddleOfTypingANumber = true
 }
 }
 @IBAction func enter()
 {
 userIsInTheMiddleOfTypingANumber = false
 if let result = brain.pushOperand(displayValue){ displayValue = result }
 else {displayValue = 0}
 }
 @IBAction func operate(sender: UIButton)
 {
 if userIsInTheMiddleOfTypingANumber{ enter() }
 if let operation = sender.currentTitle
 {
 if let result = brain.performOperation(operation) {displayValue = result}
 else {displayValue = 0}
 }
 }
 // Linked to the "C" button in the Storyboard
 @IBAction func clear()
 {
 userIsInTheMiddleOfTypingANumber = false
 brain.clear()
 displayValue = 0
 }

Model

import Foundation
class CalculatorBrain
{
 private enum Op: Printable
 {
 case Operand(Double)
 case UnaryOperation(String, Double -> Double)
 case BinaryOperation(String, (Double, Double) -> Double)
 var description: String
 {
 get
 {
 switch self
 {
 case .Operand(let operand): return "\(operand)"
 case .UnaryOperation(let symbol, _): return symbol
 case .BinaryOperation(let symbol, _): return symbol
 }
 }
 }
 }
 private var opStack = [Op]()
 private var knownOps = [String: Op]()
 init()
 {
 knownOps["×ばつ"] = Op.BinaryOperation("×ばつ", *)
 knownOps["÷"] = Op.BinaryOperation("÷") { 1ドル / 0ドル }
 knownOps["+"] = Op.BinaryOperation("+", +)
 knownOps["−"] = Op.BinaryOperation("−") { 1ドル - 0ドル }
 knownOps["√"] = Op.UnaryOperation("√", sqrt)
 }
 private func evaluate(ops: [Op]) -> (result: Double?, remainingOps: [Op])
 {
 if !ops.isEmpty {
 var remainingOps = ops
 let op = remainingOps.removeLast()
 switch op {
 case .Operand(let operand):
 return (operand, remainingOps)
 case .UnaryOperation(_, let operation):
 let operandEvaluation = evaluate(remainingOps)
 if let operand = operandEvaluation.result
 {
 return (operation(operand), operandEvaluation.remainingOps)
 }
 case .BinaryOperation(_, let operation):
 let op1Evaluation = evaluate(remainingOps)
 if let operand1 = op1Evaluation.result
 {
 let op2Evaluation = evaluate(op1Evaluation.remainingOps)
 if let operand2 = op2Evaluation.result
 {
 return (operation(operand1, operand2), op2Evaluation.remainingOps)
 }
 }
 }
 }
 return (nil, ops)
 }
 func evaluate() -> Double?
 {
 let (result, remainder) = evaluate(opStack)
 println("\(opStack) = \(result) with \(remainder) left over")
 return result
 }
 func pushOperand(operand: Double) -> Double?
 {
 opStack.append(Op.Operand(operand))
 return evaluate()
 }
 func performOperation(symbol: String) -> Double?
 {
 if let operation = knownOps[symbol] { opStack.append(operation) }
 return evaluate()
 }
 func clear()
 {
 opStack = [Op]()
 evaluate()
 }
}

And my log output is:

[8.0] = Optional(8.0) with [] left over
[8.0, 9.0] = Optional(9.0) with [8.0] left over
[8.0, 9.0, ×ばつ] = Optional(72.0) with [] left over
[8.0, 9.0, ×ばつ, 2.0] = Optional(2.0) with [8.0, 9.0, ×ばつ] left over
[8.0, 9.0, ×ばつ, 2.0, −] = Optional(70.0) with [] left over
[8.0, 9.0, ×ばつ, 2.0, −, 5.0] = Optional(5.0) with [8.0, 9.0, ×ばつ, 2.0, −] left over
[8.0, 9.0, ×ばつ, 2.0, −, 5.0, ÷] = Optional(14.0) with [] left over
[] = nil with [] left over

I am pretty sure the last line in the log output is correct. Basically I was just hoping someone with more iOS/MVC experience could tell me whether or not I implemented the clear functionality correctly. If I didn't, how might I go about fixing my code?

• steveclark: Okay, it's just a lot of code for a small function. The only code I wrote is the two clear functions in the Model and the Controller. Everything else was provided in the lecture demo from Stanford.
• steveclark: I don't have it on GitHub yet. The code isn't that long I guess, so I just added it in.

Answer 1

func clear()
{
 opStack = [Op]()
 evaluate()
}

I'm just going to review this method for now. You said in comments that most of this code was provided from a lecture and the primary task was implementing this method.

First of all, calling evaluate() seems particularly unuseful. evaluate() doesn't actually change anything about the state of the operation stack. All we get out of this is the println statement. Given that this is an iOS app, println is going to be just wasted processor time (and should be eliminated from release builds). The only reason to call evaluate is if you're going to do something with its return value.

---

Additionally, rather than creating a new array, why don't we just empty the existing array?

opStack.removeAll()

---

So, our final implementation should either care or not care about what evaluate() returns.

If we choose to care, we should make evaluate() more like the other methods were we are pushing buttons other than clear:

func clear() -> Double? {
 opStack.removeAll()
 return evaluate()
}

In this case, evaluate() and therefore clear() should always return nil, but it might be useful for unit testing to go ahead and return the value. We can always assert the nil-ness in unit testing.

assert(clear() == nil, "clear() must return nil.")

If we choose to not care, calling evaluate is unnecessary:

func clear() {
 opStack.removeAll()
}

---

ADDENDUM: I'm also going to argue in your view controller that we shouldn't ever need to write the line displayValue = 0. I know that it happens throughout the provided code, but I don't consider it all that great. In all cases with the if let/else, it would be simply to just write:

displayValue = brain.performOperation(operation) ?? 0

instead of the big if let/else which we've smushed down to save lines.

In the case of your implementation of the clear() @IBAction method, it's fine as is if we leave our clear() with no return value. However, if we give it a return value, we should repeat our use of Swift's nil-coalescing operator:

displayValue = clear() ?? 0

Answer 2

(Also a beginner following Stanford CS193p 2015.)

Two slightly different solutions here somewhere between steveclark's and nhgrif's. My @IBACTION in ViewController code is the same in both of my versions. In each case the newValue of displayValue is being returned as follows -

In ViewController:

@IBAction func cancel() {
 displayValue = brain.cancel()
}

In CalculatorBrain:

 func cancel() -> Double {
 opStack = [Op]()
 return 0
 }

In the same lecture we are also tasked with adapting the code so that displayValue is a Double? - and it knows what to do with a nil value. So in another version of my answer to the same question which steveclark is addressing here I had the following -

In CalculatorBrain:

func cancel() -> Double? {
 opStack = [Op]()
 return evaluate()
}

The second version returns a nil which the diplayValue set knows what to do with.

• nhgrif: return 0 isn't very useful in the first attempt. Instead, perhaps we should return evaluate() ?? 0 which will make sure evaluate() isn't returning a number. The ?? 0 allows us to keep the return type as Double instead of Double? as it'll return 0 any time evaulate() gives us nil.
• nhgrif: I'm also going to suggest you take a look at my answer in which I'm calling removeAll() on the array rather than creating a new array.
• nhgrif: With all that said though, welcome to Code Review. This is a very good first answer.
• simons: Have already incorporate removeAll() into my code. Thanks for that.
• nhgrif: Not in the answer here? That's what I was pointing at.