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
asked Aug 10, 2015 at 1:47
\$\endgroup\$
0

1 Answer 1

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.

answered Aug 10, 2015 at 12:46
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.