3
\$\begingroup\$

Today I created my second app - a simple body mass index calculator. The BMI is calculated by the following formula:

$$\dfrac{\text{weight}}{\text{(height)}^2}$$

The general meaning of the resulting BMI values:

  • \$<18.5\$ = underweight
  • \$18.5-25\$ = normal weight
  • \$25-30\$ = overweight
  • \$>30\$ = obese

My goal was to improve my skills and make something useful at the same time. I'm sharing my code in hope to get some constructive feedback. Is there anything I could improve?

My model file:

import Foundation

struct CalculatorBrain {
let weightInKG: Double
let heightInCM: Double

init(weightInKG: String, heightInCM: String) {
    self.weightInKG = Double(weightInKG) ?? 0.0
    self.heightInCM = Double(heightInCM) ?? 0.0
}

func calcBmi() -> Double {
    return weightInKG / ((heightInCM / 100) * (heightInCM / 100))
}
}

The ViewController:

class ViewController: UIViewController {

@IBOutlet weak var weightTextField: UITextField!

@IBOutlet weak var heightTextField: UITextField!

@IBOutlet weak var resultTextField: UILabel!

@IBAction func calculateBtn(_ sender: Any) {
    guard let weightInKG = weightTextField.text,
        let heightInCM = heightTextField.text else {
            return
    }
    let cm = CalculatorBrain(weightInKG: weightInKG, heightInCM: heightInCM)
    let result = cm.calcBmi()
    resultTextField.text = "BMI: \(result)"
}

}
\$\endgroup\$
1
\$\begingroup\$

Let me just congratulate you on some really good naming! It's a great idea to put the units in the variable name. I just spent the better part of a day on a tight deadline chasing down a bug that ended up being a units issue. I felt so stupid. The only named I'd work on improving is result. Why not just call it bmi? result is a very generic name unless you're writing something like a square root function. It doesn't tell you what it's the result of.

It seems a little weird to me that init takes its parameters as Strings and immediately converts them to Double. Why not have the model object just take them as Double and let the controller convert them? If you ever reuse this class or expand it for a task where the data doesn't come from a text field (like maybe it comes from a database or the health app), you may not have the values as Strings. It seems most efficient to have the init method take them in the format they'll be stored in and let the caller worry about doing the conversion.

Indentation styles are a personal thing. My preference would be to indent everything between the open and close braces of a struct or class. It's a minor thing, but I feel it improves readability, especially if you end up needing to put more than one in the same header.

\$\endgroup\$
  • \$\begingroup\$ Hi @user1118321 ! Thank you very much for your feedback. This really helps me to improve, especially your idea on how I can improve my init with the type conversion. \$\endgroup\$ – JVermeer May 2 '17 at 12:55
3
\$\begingroup\$

I have some minor tips about naming:

  • weight is entirely different than mass, thus I advice you to change that, because some users might be annoyed by this common confusion, if it is present in the user interface of the app. See Mass versus weight

  • CalculatorBrain does not really feel related to BMI. Maybe BMICalculator would be a better fit here


When it comes to your struct, if that is all you are planning to do with it, this gets quite useless. Instead, I would simply use a function, taking two Double parameters (to avoid String conversion). Also, I would not use ((heightInCM / 100) * (heightInCM / 100)), because it does an unnecessary multiplication between two known numbers, two of 100 in this case. This would be an example:

func calculateBMI(massInKilograms mass: Double, heightInCentimeters height: Double) -> Double {
    return mass / ((height * height) / 10000)
}

It is shorter and, in my opinion, more elegant and efficient. And now you do not need to initialize a struct and call one of its methods each time, instead you only need to call it:

calculateBMI(massInKilograms: x, heightInCentimeters: y)
\$\endgroup\$
  • \$\begingroup\$ Hey, welcome to Code Review! This looks like a fine first answer. I see you have already read through the tour, so there's nothing left to do for me here. \$\endgroup\$ – Graipher May 3 '17 at 8:17
  • \$\begingroup\$ Thank you @Graipher! Being used to the other Stack Exchange sites helps. \$\endgroup\$ – Mr. Xcoder May 3 '17 at 10:24
  • \$\begingroup\$ Hi @Mr.Xcoder, I really like your tips. I thought about changing ((heightInCM / 100) * (heightInCM / 100)) as well because like you already said it doesn't look elegant at all. Now I have a way to improve this part of my code which I'm really thankful for. Have a nice day! \$\endgroup\$ – JVermeer May 3 '17 at 17:17

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.