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

Clear function for Stanford iOS 8 Calculator application

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

Problem

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
{

Solution

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

Code Snippets

func clear()
{
    opStack = [Op]()
    evaluate()
}
opStack.removeAll()
func clear() -> Double? {
    opStack.removeAll()
    return evaluate()
}
assert(clear() == nil, "clear() must return nil.")
func clear() {
    opStack.removeAll()
}

Context

StackExchange Code Review Q#84866, answer score: 4

Revisions (0)

No revisions yet.