3
\$\begingroup\$

I have a switch statement with five cases, and four of them are really similar to the others. How could I properly refactor this code to eliminate the duplication of code?

func appendOperand(operand: Operand) {
    let displayedString = amountLabel.text!
    if displayedString.last != decimalSeparator {
        switch operand {
        case Operand.Div:
            if division {
                dropLastChar()
            } else {
                if multiplication || subtraction || addition {
                    dropLastChar()
                }
                appendOperand(buttonDivision, operand: &division, operandName: "/")
            }
        case Operand.Mult:
            if multiplication {
                dropLastChar()
            } else {
                if division || subtraction || addition {
                    dropLastChar()
                }
                appendOperand(buttonMultiplication, operand: &multiplication, operandName: "x")
            }
        case Operand.Sub:
            if subtraction {
                dropLastChar()
            } else {
                if division || multiplication || addition {
                    dropLastChar()
                }
                appendOperand(buttonSubtraction, operand: &subtraction, operandName: "-")
            }
        case Operand.Add:
            if addition {
                dropLastChar()
            } else {
                if division || multiplication || subtraction {
                    dropLastChar()
                }
                appendOperand(buttonAddition, operand: &addition, operandName: "+")
            }
        default:
            break
        }
    }
}

func appendOperand(button: UIButton, inout operand: Bool, operandName: String) {
    setAllOperatorsToFalse()
    operand = true
    wasDecimalAmount = isDecimalAmount
    isDecimalAmount = false
    setDisplayWith(operandName)
}

func dropLastChar() {
    isDecimalAmount = wasDecimalAmount
    setAllOperatorsToFalse()
    let displayedString = dropLast(amountLabel.text!)
    amountLabel.text! = displayedString
    calculatorMode = displayedString.rangeOfString("[x/+-]", options: .RegularExpressionSearch) != nil
}

func setAllOperatorsToFalse() {
    division = false
    multiplication = false
    subtraction = false
    addition = false
    equal = false
}

enum Operand {
    case Div, Mult, Sub, Add, Equal
}

I thought at first making a method that would do everything that is in each case but then I would end up with a lot of arguments for the method.

Another problem I'm facing is the multiplication || subtraction || addition part. If addition == true, multiplication, subtraction and addition are false for example. I am sure there is a more elegant way to do this.

EDIT:

So here is my calculator:

enter image description here

Every time one of the operand (x, +, / or -) is pressed, appendOperand(operand: Operand) is called:

@IBAction func buttonPressed(sender: UIButton) {
    switch sender.tag {
        // operands
    case Tag.Add.rawValue:
        appendOperand(Operand.Add)
    case Tag.Sub.rawValue:
        appendOperand(Operand.Sub)
    case Tag.Mult.rawValue:
        appendOperand(Operand.Mult)
    case Tag.Div.rawValue:
        appendOperand(Operand.Div)
    case Tag.Equal.rawValue:
        appendOperand(Operand.Equal)

    default:
        break
}

It adds the operand at the end of the string displayed in the label ("96x" in my example on the screenshot).

I use these flags for each operator to manage if whether or not the operand button has been pressed before. For example, if I press x, then x will appear on the label. If / was pressed right before, / will be removed from the label before adding x. Or if I pressed x and press it again, x will be removed from the label as if I never pressed it.

These operator are declared on top of the UIViewController:

var addition: Bool = false
var subtraction: Bool = false
var multiplication: Bool = false
var division: Bool = false
var equal: Bool = false
\$\endgroup\$
2
\$\begingroup\$

So, as a start, I'd take my enum, which you've stuck at the bottom, and move it to the top. It's great to use custom types, but sticking it at the bottom of the file does a disservice to readers. And the fact that it has been placed at the bottom makes me think that the author may have undervalued this custom type (and judging by the code, I'd say the enum certainly appears undervalued).

So:

  1. Move the enum to the top so that future readers can start there.

Next, what are we gaining by abbreviating the operand types? Not a whole lot. We only have to fully type it out once, and the rest of the time it will autocomplete for us, right? Right. By fully typing out the name the first time, we're going to increase the readability throughout everywhere that uses the enum. And writing readable code is important.

So:

  1. Move the enum to the top so that future readers can start there.
  2. Don't abbreviate.

Finally, look at what we're doing. We call our appendOperand and pass in one of our Operand enum values, and that does some work to eventually call an overloaded appendOperand method that takes a string. A string which we're magically associating with the operand, and only doing so in this very weird way. But why? Swift enums can have a backing value associated with them (and there's no performance hit either for storing them or for passing them around... they still perform the same). We can access that backing value using the rawValue property of enums.

So:

  1. Move the enum to the top so that future readers can start there.
  2. Don't abbreviate.
  3. Use a backing value rather than magically associated strings.

Applying these three points gives us the following enum at the top of the file:

enum Operand: String {
    case Multiply = "x"
    case Divide = "/"
    case Add = "+"
    case Subtract = "-"
    case Evaluate = "="
}

Now, we take a close inspection of the appendOperand method, and I notice a few oddities...

  1. We take a UIButton argument, but we don't use it.
  2. We take an inout argument and return void (hint: don't take the argument and just return a value).
  3. Now that we've fixed our enum, we can take the enum as an argument rather than a String.

There are more issues that can and should be addressed, but getting the enum right is so fundamental to this code that we need to take a breather after fixing that up, see what we can do after fixing that up, and reassess where we're at before we start making more changes.

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.