2

I'm creating a BMR calculator, and I have a class called User. This class contains all the methods used to calculate the BMR, as well as a constructor to pack the user's data (age, gender, weight, height) together.

The code is:

public class User {

int age;
String gender; // todo: use an Enum
double height; // height stored in cm, weight in kg (so if user enters in feet/lbs, conversions are done to cm/kg and *THEN* passed through to constructor below)
double weight;
double activityMultiplier; // todo: use an Enum  (possibly)
int bmr;


public User(int age, String gender, double height, double weight,
    double activityMultiplier) {
    this.age = age;
    this.gender = gender;
    this.height = height;
    this.weight = weight;
    this.activityMultiplier = activityMultiplier;

    bmr = calcBMR();
}

/**
 * If user input is correct, this method will calculate the BMR value of the user given their input and measurement choices.
 * 
 * @param None
 * @return BMR Value
 */
public int calcBMR() {
    int offset = gender.equals("M") ? 5 : -161;
    // This is the body of the calculations - different offset used depending on gender. Conversions to kg and cm done earlier so no conversions needed here.
    // The formula for male and female is similar - only the offset is different.
    return (int) (Math.round((10 * weight) + (6.25 * height) - (5 * age) + offset)); // This is the Miffin St-Jeor formula, calculations done in cm/kg
    }

/**
 * If the user selects the TDEE option, this method will be executed after the calcBMR() method. 
 * A value from the calcBMR() method will be passed down to this method, and is multiplied
 * by the activity level parameter passed into this method.
 * 
 * @param bmr (output from calcBMR() method
 * @return TDEE Value
 */
public int calcTDEE(int bmr) {
    return (int) Math.round(calcBMR() * activityMultiplier);
}

}

My concern is that I'm not sure that the way I'm initialising the value for bmr in the constructor (bmr = calcBMR()) is correct. I can't calculate the bmr till the users age, gender, height and weight are recorded and stored in variables (which are what the 5 lines above do). Is this programming structure okay? I.e. when a User object is created, the age, gender, height and weight are stored in variables, and THEN a method is called within the constructor to calculate and store another value.

Is there a better way to do this? If not, do I need to do this.bmr = calcBMR() or is bmr = calcBMR() fine?

Note the User object is created in a seperate class. The reason I'm mainly confused is because I'm not passing a bmr parameter into the constructor, I'm using a method return value to initiate the value of an instance variable instead.

Improve this question

3 Answers 3

Reset to default
3

It's ok syntactically, however you should not call an override-able (public/protected non-final) method from your constructor. If someone overrides it, it could mess up the construction of your object. Calling helper methods from a constructor is fine, just make it private or final.

this.bmr = calcBMR() is the same as bmr = calcBMR()

Saying bmr.calcBMR() wouldn't make sense because the calcBMR method is on the User object. bmr is an int so it doesn't have a method named calcBMR

Whether you use this or not is a matter of your preference. It only really makes a difference if you have a local variable also named bmr, and then you are explicitly calling the instance variable rather than the local. In general, it's confusing to have a local and instance variable with the same name.

Your calcTDEE method is a little bit off though. You can just use the value of bmr, not pass it in or recalculate it, so it would be

public int calcTDEE() {
    return (int) Math.round(bmr * activityMultiplier);
}
Improve this answer
Sign up to request clarification or add additional context in comments.

8 Comments

Thanks for the comment. Sorry, when I wrote bmr.CalcBMR(), I meant bmr = calcBMR. Edited my post. So the only suggestion you'd make is to make the calcBMR() method final? What does this exactly do?
or more likely, make it private if you're only calling it from inside the class. by making it final or private, it means that no one else can change the behavior of it. you could create a subclass that overrides calcBMR() and then your calculations could be changed (assuming you don't want that). You could also make it private and then have a getBMR() method that lets people just get the value. I assume you don't actually want people calling calcBMR(), you just want them to get the value, is that correct?
Yeah, that is correct. The only place I call calcBMR() myself is in the constructor, and the main class just creates a User object called user1, and refers to bmr as user1.bmr. I'll make this method private as it's only being called from the User class, sound good? Thanks :)
In fact, I've left it public but made it final, so that way if I need to modify the code I could still call it from the BmrMain class. Is adding final to the method signature feasible enough to make it non-overridable?
In addition to Jeff's excellent points, I'd suggest that you make all your fields private and provide getter methods for each of them. Without this, it is possible for external code to change some of the field values without the bmr being recalculated.
|
0

I think your way is ok. However, it is better if you use all your properties is "private" and have get/set for them. So, when another class extends your class, we don't have to worry about messy things. With the calcTDEE method, I think you just need to put your bmr property inside it because it is set in your constructor. So, at the time you call this method, the bmr has the right value.

public int calcTDEE() {
    return (int) Math.round(bmr * activityMultiplier);
}

Hope this help.

Improve this answer

1 Comment

I'll look into using getters and setters and making the variables private, although I amn't very keen on using them. I have changed my calcTDEE() method to your suggestion, thanks for the reply :)
0

Better to separate the calculation from the construction.

I personally believe we should avoid doing this. According to me for any person using Class which is constructing your Object will be unaware of the fact that bmr is also being calculated in the User by seeing this particular constructor, until he look into the code. When you are building API you should make it more readable for the consumer who will going to use your User, apart from that it's totally valid. Moreover, as suggested by Jeff Storey if you don't want user Objects to use your calcBMR you should make it private.

One more point I want to add here that bmr is one of the Health related parameter of the Person so you should collect all this kind of parameters in separate class which can include bodyFat, bmr, medicalAge etc in it and pass your User Object in constructor of this class. So, ultimately Health parameters can only be constructed with valid user only.

Improve this answer

3 Comments

I like that suggestion, but I'm not quite ready to do a bodyfat calculator etc. I think when I add more options I will just store the basics in User class, and create another class that can incorporate all the methods to calculate BMR, TDEE, Body fat, Macros(?) etc, together with a constructor that can store the values. Is this what you're trying to say?
Okay, do you think the best way to do this is to call the methods from the Health class (i.e calcBodyFat(), calcBMR()) from the constructor in the User class?
Okay thanks, so I believe the way I have it right now (providing I amn't adding any more functionality, just tidying up my current code), is fine - i.e. calling a method in a constructor.

Your Answer

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