First of all, excuse me if my english isn't perfect, but I'm not from a english speaking country (Spain), so...
Well, here's the question. When creating a class, ¿is a good practice to use temporary variables all you can, or is better to declare your variables as class variables, just in order to keep things clear?
I'll give you an example, using a simple class SpriteSheet. Is a very short and popular class, used almost in every 2D game out there in Java.
Here is the code just as it originally was planned by the creator of the tutorial I was watching:
public class SpriteSheet {
private String path;
private final int SIZE;
public int[] spriteSheetPixels;
public SpriteSheet(String path, int size) {
this.path = path;
SIZE = size;
spriteSheetPixels = new int[SIZE * SIZE];
load();
}
private final void load() {
try {
BufferedImage image = ImageIO.read(SpriteSheet.class
.getResource(path));
int w = image.getWidth();
int h = image.getHeight();
image.getRGB(0, 0, w, h, spriteSheetPixels, 0, w);
} catch (IOException e) {
e.printStackTrace();
}
}
}
Point is, he is doing just a normal class, following all Java conventions as far as I know. After having a look at it, I thought I could improve it a little bit. Here is my version of the very same class:
public final class SpriteSheet {
public final int[] spriteSheetPixels;
public SpriteSheet(final String path, final int width, final int height) {
spriteSheetPixels = new int[width * height];
load(path, width, height);
}
private final void load(final String path, final int width, final int height) {
try {
BufferedImage image = ImageIO.read(SpriteSheet.class
.getResource(path));
final int w = image.getWidth();
final int h = image.getHeight();
final byte ZERO = 0;
image.getRGB(ZERO, ZERO, w, h, spriteSheetPixels, ZERO, w);
} catch (IOException e) {
e.printStackTrace();
}
}
}
Just in case, if you don't feel like paying too much attention, I'll try to resume what I've changed and why: - Added "final" to class declaration, as I don't think I'll ever need to instantiate it. - Removed all class variables except for the array, as it is the only thing I'll end up using from this class. I feel like having the rest of the variables declared as class variables is just a waste of memory. If they're temporary, if I'm not mistaken, they'll be used and then the GC will take care of them sooner or later, freeing memory. - Marked the array as final because it'll stay as it is for the rest of runtime. - Split the SIZE constant into width and height, just in case I decide using some non square sprite sheets. - Declaring w and h is actually a good think, as calling methods in the parameters is usually bad for execution speed (or that's what I've read in some places). - As 0 is used several times, I believe declaring it as a variable will help to improve the execution speed (just a tiny little bit, probably won't be noticeable anyway).
And that's basically all of it. Be aware I'm a studient and probably I'm making some very n00b mistakes, and that's why I wanted to ask here, as I'm sure there are lots of experienced programmers around.
Bare in mind I don't really care about the SpriteSheet class, I'm more curious about the quality of my optimisations.
¿Did I improved things or make them worst (making things actually slower, less readable, harder to maintain in the future, doing things that the compiler will do anyways...)?
Sorry if my question is too long and too vague, is my first one so, easy on me ;)
Thanks in advance.
EDIT:
I just read it after a little break, and it doesn't make any sense (have you seen that I'm parsing width and height parameters to load() and never use them?)
Here is as I believe it should be:
public final class SpriteSheet {
public final int[] spriteSheetPixels;
public SpriteSheet(final String path, final int width, final int height) {
final byte ZERO = 0;
spriteSheetPixels = new int[width * height];
try {
BufferedImage image = ImageIO.read(SpriteSheet.class
.getResource(path));
image.getRGB(ZERO, ZERO, width, height, spriteSheetPixels, ZERO,
width);
} catch (IOException e) {
e.printStackTrace();
}
}
}
Just realized I don't need the method really. Everything can be done in the constructor.
Assuming that you mean instance variables as per your example, rather than class / static variables. (If you did really mean static variables, then you have a whole lot of other problems to deal with ...)
Lets start with the "optimization" aspect of this Question.
The first thing to say is that the difference between the two approaches is probably insignificant. The chances are that it won't make a noticeable difference to the performance of the program. In your case, I would imagine you have at most few tens of instances of that class at any one time, so the difference in memory usage will be a few thousand bytes at most.
Having said that, there is a difference. When you declare the fields as instance fields, they will exist (and occupy heap memory) for the lifetime of the object. By contrast, local variables cease to exist when the enclosing method call ends. So using a local variable is likely to use less memory in the long term.
But the important issues here are readability, maintainability and correctness.
If you turn local "temporary" variables into instance variables, then there is scope for different methods ... or different calls to the same method ... interfering with each other via there use of the instance variables. Note that in some use-cases, the interference is unavoidable. For example, when two different threads call the same method on the same object simultaneously, or when a method directly or indirectly calls itself; i.e. recursion.
And the fact that such things could happen makes the code harder to read and harder to maintain. (Unless you are really familiar with the code, and are tracking all changes that anyone makes to it ... you can't be sure that something has broken your assumptions ... and you have to check.)
By contrast, you don't have these problems with local variables. They are guaranteed to not be visible to any other method call, on the current thread or another one.
In short, declaring variables as instance variables "just in order to keep things clear" is actually going to have the opposite effect. It is going to make things significantly less clear.
Finally, there was an interesting Question related to this on the Programmers site:
My conclusion in my Answer was this doesn't really qualify as an "anti-pattern" because it is not a design pattern. But nonetheless it is really bad.