Handling calculator keypresses

Question

I've added decimal (floating point) functionality to my calculator implementation; it's working but I'm having trouble making it more concise and less ugly.

I'm trying to use the same method to handle both number keys and the decimal point, and whichever way I look at it, I either have to check for the button being pressed and then whether the user is just starting a new entry, or the other way around. I've even drawn a flowchart to see how could this be condensed with nested logical operators (||, &&) but couldn't get it to work.

Obviously I could write a separate method for the decimal key, but then I'd get to repeating the isTyping verification there instead. Is there a simple way to do this or is code repetition inevitable at some point or another?

@IBAction func buttonDigit(sender: UIButton) {
    let digit = sender.currentTitle!

    if isTyping {
        if digit == "." {
            if !hasDecimal {
                display.text = display.text! + digit
                hasDecimal = true
            }
        } else {
            display.text = display.text! + digit
        }
    } else {
        if digit == "." {
            display.text = "0" + digit
        } else {
            display.text = digit
        }
        isTyping = true
    }
}

Answer 1

I suggest hooking up the . button to a separate IBAction. The decimal point isn't a digit, so the function name is confusing. Furthermore, the code to handle a decimal point has little in common with the code to handle a digit. There's no point in mashing the code together into the same function.

@IBAction func buttonDecimalPoint(sender: UIButton) {
    if !isTyping {
        display.text = "0."
    } else if !hasDecimal {
        display.text = display.text! + "."
    }
    isTyping = true
    hasDecimal = true
}

@IBAction func buttonDigit(sender: UIButton) {
    if !isTyping {
        display.text = ""
    }
    display.text = display.text! + sender.currentTitle!
    isTyping = true
}

Note that in buttonDecimalPoint, I test if !isTyping rather than if isTyping, so that I can write else if instead of nesting the conditionals like this:

@IBAction func buttonDecimalPoint(sender: UIButton) {
    if isTyping {
        if !hasDecimal {
            display.text = display.text! + "."
        }
    } else {
        display.text = "0."
    }
    isTyping = true
    hasDecimal = true
}

Another benefit is that the not-typing state can be thought of as the "initial" state, so it's more fitting that the code to handle it should come first.

Furthermore, I've moved the = true assignments to the end of each function to make the invariants obvious. After pressing ., you know that isTyping and hasDecimal are both true.

Answer 2

^{Please note, I've included this section at the top because it is absolutely the most important change you make in your code.}

First and foremost, we are one-hundred percent absolutely going to get rid of that horrendous ! operator. In almost all cases, the ! is just begging for trouble. UITextField's text property returns a String!, which means its return value could be nil. It is generally nil before its text has been set at all, but it can also always be set to nil. And if it ever is nil and someone taps one of these buttons, the whole app comes crashing down.

In this case, the way out is quite simple. We will always want to handle nil as an empty String here, right? So let's use the nil-coalescing operator, ??:

display.text = (display.text ?? "") + digit

_{The fact that UITextField.text returns String! rather than String? simply means that Apple has not applied the nullability rules to UITextField yet. When they do, this property will no longer be a String!, but instead a String?. If it changes to a String?, this code is going to be exactly what we want. If, however, it changes to a String, neither this code nor your original code are going to be what we want. At that point, we'll want to see simply: display.text = display.text + digit, because it could never be nil.}

---

Now, as for the actual logic here, I think we're doing to much, and I actually disagree with 200_success's assessment of splitting these to different methods (at least in the way he suggests).

I think all of our validation logic should all be in the same place. I think hasDecimal is an unnecessary Bool to track. I'm not sure what isTyping is doing here (it seems probably unnecessary). And I think a single IBAction method can directly handle all of the button presses.

And why? Because that single method should look like this:

@IBAction func digitPressed(sender: UIButton) {
    if let digit = sender.currentTitle {
        self.appendDigit(digit)
    }
}

And that's it.

But obviously, this requires writing an appendDigit method. This method handles all of our validation logic... but that method can be broken down to handle the special cases.

---

The reason I think hasDecimal and isTyping are unimportant to track is because what is actually important is what the display string actually looks like, correct? So we need to be checking what that string actually looks like and use that as the basis on whether or not to allow another decimal to be added (or whether or not the number can be made negative, or whether or not to add a leading zero).