4

I'm really confused with the concept of static vs instance methods. I have created a BMR calculator. I've seperated the GUI from the calculations using different classes.

public class Calculations {

/**
 * If user input is correct, this method will calculate the BMR value of the user given their input and measurement choices.
 * 
 * @param userAge, userHeight, userWeight
 * @return BMR Value as a string
 */
public static int calcBMR(int age, String gender, double height, double weight) {

    // This is the body of the calculations - different formulas used depending on gender. Conversions to kg and cm done earlier so no conversions needed here.
    if (gender.equals("M")) { // Uses male bmr formula if genderMale radiobutton is selected
        return (int) (Math.round((10 * weight) + (6.25 * height) - (5 * age) + 5)); // This is the Miffin St-Jeor formula, calculations done in cm/kg
    } else { // else gender.equals("F") - there are only 2 options for gender, M or F.
        return (int) (Math.round((10 * weight) + (6.25 * height) - (5 * age) - 161));
    }
}


/**
 * 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 selectedActivityLevel
 * @return TDEE Value (as a string)
 */
public static int calcTDEE(double activityMultiplier, int bmr) {
    System.out.println(activityMultiplier);
    return (int) Math.round(bmr * activityMultiplier);
}

}

As you can see, the methods are STATIC, however the variables being passed through (to both methods) are instance variables.

I am only calling these methods through the following lines:

            bmrValue = Calculations.calcBMR(userAge, userGender, userHeight, userWeight);
            bmrLabel.setText("<html><br /><font size=4>You have a <i><font color=#ce0000>BMR</font></i> of: " + "<font color=#59AF0E>" +  bmrValue + "</font></html>");
            if (tdeeYes.isSelected()) {
                userActivityLevel = activityMap.get(activityLevelBox.getSelectedItem());
                // Looks up selected item from combo box, which is the KEY. Then looks up the value to this key from the map - this value is the TDEE multiplier.
                tdeeLabel.setText("<html><br /><font size=4>You have a <i><font color=#ce0000>TDEE</font></i> of: " + "<font color=#59AF0E>" + Calculations.calcTDEE(userActivityLevel, bmrValue) + "</font></html>");
                }

The variables are defined as:

HashMap<String, Double> activityMap;
String[] activityLevels = {"Sedentary", "Lightly Active", "Moderately Active", "Very Active", "Extra Active"};


int userAge;
String userGender;
double userHeight;
double userWeight;
double userActivityLevel;

int bmrValue;

Am I using static/instance variables correctly? Earlier I had all my parameter variables as static, as I know static methods can only access static variables. I didn't know that the parameters could be instance variables until now.

Any guidance would be appreciated.

  • 3
    Parameters can be whatever you want--methods just get references (or primitives). Where those references come from isn't relevant. As to whether or not you "should" be using instance or static methods, depends. – Dave Newton Aug 31 '15 at 05:21
  • Parameters are parameters; they are not instance variables. You are using them just fine. – dave Aug 31 '15 at 05:21
  • @dave Cheers for that. I acknowledged that after figuring out my compiler threw no errors when passing instance variables through as parameters. However, what factors are dependent on whether or not my methods should be static or instance? I'm not sure whether they should be static in this case. Is it okay for a method to be static even though they are accessing no static variables? (All my variables in my code are instance as said earlier). Cheers. –  Aug 31 '15 at 05:27
  • See this: http://stackoverflow.com/questions/2671496/java-when-to-use-static-methods – Zarwan Aug 31 '15 at 05:53
  • Can I suggest to post this on CodeReview instead of SO? I'd change more than few static methods. Btw static methods are not inherently wrong but they are often a symptom of a bad design (especially?) when collected in an utility class. – Adriano Repetti Aug 31 '15 at 06:24

4 Answers4

1

For starters, the difference between static and instance variables is that, only ONE static variable exists for all the instances of the class, whereas an instance variable exists for EVERY instance of the class.

Now, when you are talking about methods, in most cases you need to make a method static when you are trying to call it from another static method (such as main).

In general this practice is wrong for OOP, and chances are you should rethink the way the program is structured.

If you could provide more details about the code you use to call these methods, I would be able to help you with more details on how to fix this.

EDIT: In light of the new info you provided:

1) I believe that bmrMain,BMRMain and Calculations can all be merged into one class. The constructor of the new class should be adjusted to read input (as well as getters and setters for each variable for added flexibility - optional)

2) Regarding the calculation of the bmr part, there are many ways to tackle this, so I will go ahead and suggest the one that is best in my opinion. Add a Button with a text "Click to calculate" or something similar, and implement an ActionListener. The action Listener will in turn call (whenever the button is clicked) the method that calculates everything, and finally it will change the text on the JLabel.

sample code for the action Listener:

JButton button = new JButton("Text Button");
button.addActionListener(new ActionListener()
{
  public void actionPerformed(ActionEvent e)
  {
    //Call the methods for the calculation
    // bmr and tdee are temporary variables to store the results from the methods
    //handle them properly, or remove them if not needed (I just wrote a sample)
    int bmr = calcBMR(...); //pass the correct variables as arguments here
    int tdee;
    if(userHasPickedTDEE) {  tdee = calcTDEE(...); }  //and here
    label.setText(....);

  }
});

These 2 steps will take care of your "static" problem.

I recommend that you do some research on event-driven programming

And here is some extra and more in-depth reading on Action Listeners

If you need further help, or clarifications let me know :)

EDIT2:

In general, yes it is a good practice to separate classes in order to keep the code manageable. But in this case I think it is a bit superfluous to create a new class just for 2 methods. Nevertheless if you would like to keep the current structure, the way to fix the "static" is: 1) remove static from the 2 calculation methods. 2)line 332 should be 

Calculations c = new Calculations();
bmrValue = c.calcBMR(userAge, userGender, userHeight, userWeight);

FINAL_EDIT:

Well, these questions can easily be transferred to a thread of their own. But here is a useful link from a quick google search I just did, that will help demistify the static keyword:

Static keyword in Java

Mechanic
  • 172
  • 1
  • 10
  • The user enters input (age, weight, height, gender) which is then stored in variables. This is done in a class called bmrMain which collects all the input etc. I have a seperate class called Calculations, which, after collecting the data and storing them in variables, these variables are passed into methods in that class, and the output is an integer calculated from the parameters. All the data comes from a seperate BMRMain class. The setText is used for JLabel - the output value is concatenated onto the JLabel. I'm not sure if this was the type of description you were looking for. –  Aug 31 '15 at 16:00
  • Yes this new info helped me obtain a better picture about the hierarchy you have implemented and it made obvious the reasons why you tried to use static methods (keep in mind though that presenting your actual code, or at least the important parts is always better than describing it). See my edits. Also I would strongly recommend you do some research on event driven programming(suggested link above), because it will help you gain a better understanding of Java. – Mechanic Aug 31 '15 at 20:01
  • Thanks for the reply. Regarding 1) - I had bmrMain and Calculations in one class, but was suggested I keep the functionality and calculations seperate (see https://www.reddit.com/r/reviewmycode/comments/3gjlh2/java_how_can_i_make_my_code_for_bmr_calculator/). Regarding 2), this is exactly what I have done. https://github.com/F-Tahir/BMRCalculator You can find the full code here (only reason I did not post it in OP was because I thought it would be too long). If github links aren't allowed here please let me know and I will delete asap. (There's a design screenshot in readme). Any criticism? –  Sep 01 '15 at 06:15
  • thank you. Although if I did keep the static keyword in my methods, what would the side effects be? I gather that it doesn't bring any benefits making it static but does it bring any disadvantages? –  Sep 01 '15 at 20:19
  • Also, why do I need to instantiate the Calculations class to access the method? Is this always a rule - i.e. to access an instance method, I must first instantiate the class? –  Sep 01 '15 at 20:38
0

My thumb rule for using static variables goes somewhat like this:

  1. Do not use static
  2. When it is needed to use static refer to (1)

For most cases, the above thumb rule works. But then, we have standard idioms popularized by JDK, apache-commons libraries. Looking into them you find that:

  1. It is OK to have utility classes that contain all static methods.
  2. It is OK to create constants using static fields in classes. Though, for this using Enum might be a better choice.
  3. It is OK to one off utility functions that work on the same class into the class.

As for the restructuring the code goes, you should be looking at who owns the data. In this case, both the methods from Calculations class looks to be using data from the other class. Move the methods into the other class.

The only reason for existence of Calculations class should be that if you are using the methods from multiple other classes which are not related. If they are related, you try to model their relationship and see where these methods go.

Dakshinamurthy Karra
  • 5,353
  • 1
  • 17
  • 28
0

I use static whenever my class is just used to call those methods (i.e. my main method calls the static methods to set variables in the main class, etc). I believe that is the "utility" class that KDM mentioned. A small example of when I use static or utility classes:

class Utility {
  public static int add(int a, int b) {
    // You would, of course, put something besides simple addition here
    return a + b;
  }
}

class Main {
  public static void main(String[] args) {
    int a = 2, b = 3;
    int sum = Utility.add(a, b);
    System.out.println(sum);
  }
}

On the other hand, if you have a class that is closer to an actual object, with properties of its own, stay away from static. If you use static, then each instance of the class which you want to be separate objects will end up with same values.

Based on the name and functionality of your class, it appears you have a utility class. Static should be fine, but it may not be necessary. Hypothetically, though, if you wanted to use this class with multiple "people" class instances for which to calculate the BMR (where each instance is unique), then I would put calcBMR() in a person class and make it non-static so that each person has their own calcBMR().

Edit: Maybe this will help too:

Instance -> Instantiate: new Calculations().instanceMethod();

Static -> Class itself, the class's "state": Calculations.staticMethod();

17slim
  • 1,233
  • 1
  • 16
  • 21
  • I will add: I am pretty sure of my own definitions and understanding of static vs instance, but I am not an expert on their usage. When people say "rethink the structure of your code" I get confused, and think "well, then, how should I do it?" If anyone could explain or link to an explanation of how to "restructure" code to use static/instance better, please do! – 17slim Aug 31 '15 at 18:04
  • Thanks for the reply. "I use static whenever my class is just used to call those methods". Not sure I fully understand this. My calculations class is only used when the two (common) methods contained in that class are called, is this similar to what you mean by when you use static methods? If so, is this why my Calculations class is a "utility" class? Sorry if I missed something in your post, it's really late for me. I'll give it a much better read tomorrow when I'm fresh. Thanks! Thanks. –  Sep 01 '15 at 06:23
  • From a simple google search (should have googled when I was initially confused): "In computer programming, a utility class is a class that defines a set of methods that perform common, often re-used functions. Most utility classes define these common methods under static (see Static variable) scope." I strongly believe after reading this that my Calculations class is a utility class as all it contains is two static methods that are commonly used (atleast once every iteration of the program run cycle). –  Sep 01 '15 at 06:26
0

If you think in terms of, do I want instance methods or static methods, you're going to be lost. Java isn't method-oriented, it's object-oriented. So for your functionality, decide whether you want an object or a pure function (where pure function means no dependencies and no side-effects, it's strictly a function that given this returns that).

It might be good to model this as an object, since you have information describing some aspect of the User and it may make sense to keep it together:

public class User {
    private int age;
    private String gender; // todo: use an Enum
    private double height;
    private double weight;
    private String activityLevel; // todo: use an Enum

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

    public double calculateBmr() {
        int offset = gender.equals("M") ? 5 : -161;
        return (int) (Math.round((10 * weight) 
            + (6.25 * height) - (5 * age) + offset));
    }
}

etc. This way we don't have to preface the variables with "user", they're all placed together in one object. Objects are an organizational scheme for collecting related information, the instance methods are functions that use the object's information.

The alternative route is to create a utility class:

public final class UserCalcUtil {
    private UserCalcUtil() {} // no point instantiating this

    public static int calculateBmr(String gender, double height, 
    double weight, int age) {
        int offset = gender.equals("M") ? 5 : -161;
        return (int) (Math.round((10 * weight) 
            + (6.25 * height) - (5 * age) + offset));            
    }
}

Here you're keeping track of the user data separately, and passing it in to the static method when you want to calculate something. The utility class is a dumping ground for separate static methods (which you can think of as functions).

Whether you want to use objects or utility classes depends on how you want to organize your data and do calculations on it. Packing the data for a User together with the methods that act on that data may be more convenient if you always do the calculations on the individual objects, or moving the calculations into a utility class might make sense if there are multiple unrelated classes that you want to do calculations for. Since Java has more support for object-oriented constructs it often makes sense to take the object approach.

And you can use static methods, but avoid static variables for anything but constants (public static final, with a type that's immutable).

Nathan Hughes
  • 94,330
  • 19
  • 181
  • 276
  • Thanks for the reply. Isn't my calculations class already sort of a utility class? (Even though they contain instance methods rather than static). Offtopic - can you explain how the following line works: int offset = gender.equals("M") ? 5 : -161; Thanks. –  Sep 02 '15 at 02:22
  • I think I figured it out: `if (gender.equals("M") { offset = 5; } else { offset = -161; }` –  Sep 02 '15 at 02:24
  • I like the idea of the user class - if I have the instance variables as you have them in your User class, do I still need to have them in my bmrMain class? –  Sep 02 '15 at 02:31
  • @Ftahir192: yes, the conditional operator sets the variable on the left side to one thing or the other, depending on the test condition. you could put setters on the User class, or have local variables in the main method. i used the constructor here because it takes up less space and seems easier to read here than a bunch of getters and setters. – Nathan Hughes Sep 02 '15 at 02:40
  • I just updated my github with the new code and was wondering if you'd like to take a look? Mainly just the user class and lines 311 to 329 of bmrMain. I implemented some of your changes but did it slightly different to how you just suggested. I figured I still needed the variables in the bmrMain class because I need to use them to instantiate a User object (unless there's another way I'm missing). Do you think it's still a valid way? The github is: https://github.com/F-Tahir/BMRCalculator Thanks in advance if you can. –  Sep 02 '15 at 03:21