Why is it bad practice to store calculated values in Java classes, and how can I avoid it?

I'm new to Java, and I am in a graduate program learning the language. It's only been a few weeks. I got back a lab I just worked on, and the professor told me "we're not going to store calculated values in our classes" and referred me to a Best Practices document. I looked at the document, but I don't see a reference to calculated values.

I think he is referring to this piece of code:

public double getCalories() {
	this.calories = (((this.weight * 0.12) * 9) + ((this.weight * 0.09) * 4)
			+ ((this.weight * 0.02) * 4));
	return this.calories;
}

I had to use the weight to calculate the amount of calories. This is the only real calculation I am doing in this class, so I assume this is the problem. Is there a better way to write this? How can you perform the calculation then?

You'd have to ask your prof, that is not exactly front and center in the 'dos and donts of java' list. However, for your specific snippet, perhaps your prof is getting at a slightly different point:

There is absolutely no purpose whatsoever to what you are doing here, or, if there is, your code is broken.

There are only two options:

1. This is the one and only place in your entire codebase that refers to the calories field, or

2. There are other places where you use that field.

option 1 - this is the only place you use it.

Then it's useless. You calculate the calories, every time someone calls getCalories(). Java is not voodoo magic; if a method is called, each line in it is executed in order. Adding a field like this just.. also means java will store that result in that field. It doesn't mean that java will skip the calculation next time!

So, your getCalories() call will do the calculation, store the result AND return the result. The stored result is not used anywhere, and is a total waste of space. Fix: Just.. don't store it. Delete the field. Make that method a oneliner (replace this.calories = with return .

option 2 - you use it elsewhere

let's say you have another method in this class:

public boolean exceedsRecommendedDaily() {
    return this.calories > 2000;
}

then this code is BROKEN - if I invoke this method, then the calories field is still 0 and will remain 0 until someone calls getCalories(). I guess we can fix it by documenting this behaviour like so:

/**
 * Calculates if this food item on its own exceeds recommended daily intake.
 * NB: If you haven't called `getCalories()` earlier on this object,
 * this method will straight up lie to you!
 */

but I think we can all agree that means the method is idiotic.

No, why not just do it like this:

public boolean exceedsRecommendedDaily() {
    return getCalories() > 2000;
}

tada. No need for silly caveats any more.

So.. is it a best practice?

No, it is not. IF the calculation takes long enough, and the result of the calculation is needed often enough, and the object is immutable (has no set methods / none of the fields can ever change after construction) or its worthwhile for every field update to also clear out a cached value, then it is a good idea to cache the value.

For example, java's very own java.lang.String caches the hashcode, because calculating that is quite an expensive operation (it at least requires inspecting every character. So in a string of 1 million characters, that takes a while!), it can be called a ton, and strings are immutable.

Your function computes calories and returns the computed value. There's no reason in your posted code for storing a copy of the returned value; it is nowhere used.

public double getCalories() {
    return (((this.weight * 0.12) * 9) + ((this.weight * 0.09) * 4)
            + ((this.weight * 0.02) * 4));
}

If it so happens that there is other code, not shown, that uses the value of this.calories, then the getCalories method has another flaw. It is called 'getCalories' and it is surprising if it saves away the value for some other method; that other method will likely malfunction if getCalories is not called first.

"We're not going to store calculated values in our classes",
I personally think this statement is not clear, given that there is no context behind it. I guess you instructor means not storing redundant information to global field.

When creating a class, you are going to encapsulate data inside it for different reasons, you can look more at the concept of OOP language for details. In your case, your class hold information of an entity.

For example:

class car {
  int engineVersion
  String modelName
}

You see, engineVersion and modelName are important information of object car when it is initilized, so you can access it later on. When you store information, it will certainly cost space. However, for you method getCalories, assuming your own class doesn't need it (i.e: Not using it elsewhere), you can just directly return a result, then you can save resource.

Your "professor" is wrong: There is no "best practice" governing this.

The answer is "it depends".

If the calculation is expensive, consider storing it, but you'll need hooks in setters to invalidate the calculation if state changes. This is a form of caching. It complicates the class somewhat, but can be done safely and is justified if sacrificing simplicity for performance is acceptable.

If the class is immutable, you don't need to worry about state changing, so you can calculate it, ideally lazily (ie on demand), once and store the value without having any such hooks.

If the calculation is cheap, calcualting it every time is cleaner.