2
\$\begingroup\$

I've written a model for an app that generates math facts kids have to solve by evaluating the operation and the difficulty you pass to it at initialization.

I don't like how I am repeating the code that performs the operations. For instance, performAddition() and performMultiplication() both perform the same thing. How can I make this model better?

import UIKit

class QuestionGenerator: NSObject {

    enum BinaryOperation {
        case Addition
        case Subtraction
        case Multiplication
        case Division
    }

    enum Difficulty {
        case Easy
        case Intermediate
        case Difficult
    }

    struct Settings {
        var binaryOperation: BinaryOperation
        var difficulty: Difficulty
    }

    private var brain: Settings
    var number1: Int = 0
    var number2: Int = 0
    var answer: Int = 0

    init(operation: BinaryOperation, difficulty: Difficulty) {
        brain = Settings(binaryOperation: operation, difficulty: difficulty)
        super.init()
    }

    func newQuestion() {
        switch brain.binaryOperation {
        case .Addition:
            performAddition()
            break
        case .Multiplication:
            performMultiplication()
            break
        default: break
            // operation not implemented yet
        }
    }

    private func performAddition() {
        switch brain.difficulty {
        case .Easy:
            number1 = Int.random(1...10)
            number2 = Int.random(1...10)
        case .Intermediate:
            number1 = Int.random(10...100)
            number2 = Int.random(10...100)
        case .Difficult:
            number1 = Int.random(109...999)
            number2 = Int.random(109...999)
        }
        answer = number1 + number2
    }

    private func performMultiplication() {
        switch brain.difficulty {
        case .Easy:
            number1 = Int.random(1...10)
            number2 = Int.random(2...4)
        case .Intermediate:
            number1 = Int.random(1...50)
            number2 = Int.random(3...7)
        case .Difficult:
            number1 = Int.random(10...100)
            number2 = Int.random(4...15)
        }
        answer = number1 * number2
    }
}
\$\endgroup\$
3
\$\begingroup\$

Disclaimer, I'm a level Swift programmer, so take my feedback with a grain portion of salt.

First off, a minor improvement for your newQuestion function, you can leave out the breaks, in Swift, cases don't fall through by default:

func newQuestion() {
    switch brain.binaryOperation {
    case .Addition:
        performAddition()
    case .Multiplication:
        performMultiplication()
    default: break
        // operation not implemented yet
    }
}

Secondly, I see no reason you need to put binaryOperation and difficulty into a Settings struct. Furthermore, from what I can see, they should be final. I'd suggest you rewrite thing to:

private let binaryOperation: BinaryOperation
private let difficulty: Difficulty

...

init(operation: BinaryOperation, difficulty: Difficulty) {
    self.binaryOperation = operation
    self.difficulty = difficulty
    super.init()
}

Then, to generalise your perform functions, I'd add two functions to your BinaryOperation enum:

enum BinaryOperation {
    case Addition
    case Subtraction
    case Multiplication
    case Division

    func rangesForDifficulty(difficulty: Difficulty) -> (Range<Int>, Range<Int>) {
        switch self {
        case .Addition:
            switch difficulty {
            case .Easy: return (1...10, 1...10)
            case .Intermediate: return (10...100, 1...100)
            case .Difficult: return (109...999, 109...999)
            }
        case .Subtraction:
            switch difficulty {
            case .Easy: return (1...10, 1...10)
            case .Intermediate: return (10...100, 1...100)
            case .Difficult:  return (109...999, 109...999)
            }
        case .Multiplication:
            switch difficulty {
            case .Easy: return (1...10, 2...4)
            case .Intermediate: return (1...50, 3...7)
            case .Difficult: return (10...100, 4...15)
            }
        case .Division:
            switch difficulty {
            case .Easy: return (1...10, 2...4)
            case .Intermediate: return (1...50, 3...7)
            case .Difficult: return (10...100, 4...15)
            }
        }
    }

    func apply(number1: Int, _ number2: Int) -> Int {
        switch self {
        case .Addition:
            return number1 + number2
        case .Subtraction:
            return number1 - number2
        case .Multiplication:
            return number1 * number2
        case .Division:
            return number1 / number2
        }
    }
}

This allows you to write a single the newQuestion function much more elegantly:

func newQuestion() {
    let (range1, range2) = binaryOperation.rangesForDifficulty(difficulty)
    number1 = Int.random(range1)
    number2 = Int.random(range2)
    answer = binaryOperation.apply(number1, number2)
}

Some people might argue, that the BinaryOperation enum has gotten unwieldy now and they might be right. You could consider putting the rangesForDifficulty function somewhere else to alleviate this, I do think that the apply function should stay where it is though.

\$\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.