Opinions on static ArrayLists for objects (where to store them)

So, I’m working on an animation class that loads all the objects into an ArrayList right now, and really the best place I can think to store an ArrayList of said object, is within the object class itself. I’m curious if you guys think this is a good practice, or if I should put the ArrayList somewhere else?

It’s a simple little class, all it’s for is handling the animated tiles on my TiledMap, basically it loads a MapAnimations object into the MAP_ANIMATIONS ArrayList, but the catch is, that ArrayList MAP_ANIMATIONS is actually inside the MapAnimations object/class itself. It’s also should be noted it’s a static ArrayList that is dumped/reloaded when you change maps.

It works like a champ, and seems the most straightforward and simple way to do it, but at the same time feels awkward. What are your opinions on this? Should I go ahead and pull the ArrayList out of the object and put it in my MapController class?

To load a new animation, it seeks out any tiles with animation properties in the MapController class when the map is loaded:


for (int x = 0; x < map.getWidth(); x++) {
	for (int y = 0; y < map.getHeight(); y++) {
		for (int l = 0; l < tileID.length; l++) {
			tileID[l] = map.getTileId(x, y, l);
			//BUILD ANIMATIONS
			if (!(map.getTileProperty(tileID[l], "animation", "false").equals("false"))){
				MapAnimations.MAP_ANIMATION_ARRAY.add (new MapAnimations(x,y,map.getTileProperty(tileID[l], "animation", "false")));
			}
		}
	}
}

Then, the MapAnimation class itself (Don’t mind the code, it’s really rough at the moment, I wrote it all up in about 5 minutes just to test the concept :o )


package core;

import java.util.ArrayList;

import org.newdawn.slick.Animation;
import org.newdawn.slick.Color;
import org.newdawn.slick.Image;
import org.newdawn.slick.SlickException;
import org.newdawn.slick.SpriteSheet;

public class MapAnimations {

	private Animation animation;
	int duration = 195;
	private int spriteId;
	private int spriteX,spriteY;
	
	//STATIC VALUES
	private static boolean firstTime = true;
	private static ArrayList<SpriteSheet> MAP_ANIMATION_SHEET_ARRAY = new ArrayList<SpriteSheet>();
	static ArrayList<MapAnimations> MAP_ANIMATION_ARRAY = new ArrayList<MapAnimations>();
	
	public MapAnimations(int x, int y, String file) throws SlickException{
		firstTimeLoad();
		spriteX = x*32;
		spriteY = y*32;
		
		for (int x1 = 0; x1 < MAP_ANIMATION_SHEET_ARRAY.size(); x1++) {
			if (MAP_ANIMATION_SHEET_ARRAY.get(x1).getResourceReference().equals("res/animations/"+file+".png")){
				spriteId = x1;
				break;
			}
			if (x1 == MAP_ANIMATION_SHEET_ARRAY.size()-1){
				MAP_ANIMATION_SHEET_ARRAY.add(new SpriteSheet("res/animations/"+file+".png", 32, 32, 0));
				spriteId = x1;
			}
		}
		Image[] animArray = {MAP_ANIMATION_SHEET_ARRAY.get(spriteId).getSprite(0,0),MAP_ANIMATION_SHEET_ARRAY.get(spriteId).getSprite(1,0),MAP_ANIMATION_SHEET_ARRAY.get(spriteId).getSprite(2,0),MAP_ANIMATION_SHEET_ARRAY.get(spriteId).getSprite(3,0)};
		animation = new Animation(animArray, duration, true);
	}	

	public void render(float mapX, float mapY) throws SlickException{
		animation.draw(spriteX+mapX,spriteY+mapY);
	}	
	
	public void firstTimeLoad() throws SlickException{
		if (firstTime){
			System.out.println("Loading first time");
			MAP_ANIMATION_SHEET_ARRAY.add(new SpriteSheet("res/animations/barLampLeft.png", 32, 32, 0));
			firstTime = false;
		}
	}	
}

It feels awkward ? Alright, let be guided by your feelings…

  • As a rule of thumb, let classes do one thing, means have one for rendering animations, one for loading.
  • The first animation is implicitly loaded, all others must be specified externally. Why ? Do not store resource paths and names inside, rather pass them in.
  • Upper case names are usually seen only for constant fields.
  • Let the number of animation frames be flexible.
  • Put all members in private scope. MAP_ANIMATION_ARRAY is an implemention detail that should not be exposed.
  • As a beginner, do not use static. Ever.

The sheets are loaded as needed within the class, when an object is added it’s checking to see if the sheet exists, if not, it loads them.

That’s just to fill the array with at least one sheet off the bat, for testing purposes. Not going to be in the final code. The rest are loaded on-the-fly as they are requested, that way I don’t need to load thousands of animations and only use a few dozen.

My mistake, I was thinking all statics were uppercase, not just constants. I just rechecked the naming conventions and realized I’m wrong. Hmm…

Already planned, just not part of the test code. Eventually the number of frames and delay will be determined based on the sheet loaded.

Final product will have it private.

There’s nothing wrong with statics, they have their place. I think this is a good place. Why wouldn’t I want it static? I only want one array to exist, ever. I could make it non-static if I pull it out of the object and make a resource loader, I just don’t understand why I’d want to other than to make all the “statics are evil always” programmers happy.

But, to clarify your answer to my actual question; Basically I should stick the ArrayList in a separate resource loader class all by itself?

If you make something static, you leave the main path, so the question should be why instead of why not ?
For reading up on problems with statics, just do some research.
Why pull it out ? Because of the mentioned one-task-per-class rule for instance.

In a separate resource loader, it shouldn’t be static either. Otherwise you could have multiple loaders operating on the same animations list. To prevent that, you could make a singleton of the loader - only to introduce another bad practice.
Besides that, it is perfectly valid to have several loaders keeping their own private animations. You don’t have to do that, but to restrict class usage with no benefit by (bad) design makes no sense. And you never know what might be needed some day.

Hrm, I see what you’re saying. What you described is similar to how my entities system works, although it also uses a static entities arrayList to keep track of everything. But really, I kept it static under the philosophy of “why not?” since I only ever want that single array to exist.

There is another culprit with the presented approach: you can’t use the class (safely) without looking at the implementation details, which should usually not be the case.

What holds this array? Does it do one job and only instantiated once?

Why make the array static when you can just use a dreaded singleton? I know that design pattern is frowned upon but it really is an easy and cheap solution.

I use static all the time. I make references public all the time. Hiding implementation details is of no importance when you know the implementation isn’t going to need to evolve. Likewise if “correcting” a bad choice simply requires summoning the refactoring fairy (Thanks Cas I like that mental image).

I use static and public members all the time. If you know how to refactor, you don’t have to make the code structure perfect the first time around.

There’s nothing inherently wrong with static per-se when used correctly, e.g. immutable constants.

However using static class members to ‘store’ data is essentially akin to global variables, a more OO (and considerably tidier) approach could be:

  • encapsulate the MapAnimation into it’s own class (probably immutable once each one is loaded?).
  • re-factor the posted code so that it returns the animations.
  • create another model class that contains the array of map animations (may also include other game data?)
  • the controller that you mention uses that model, updates whatever needs updating, renders, etc.

Each class now has a clear and concise purpose (easier to comprehend or modify), can more easily be tested, and no nasty global variables.

  • stride

There’s nothing wrong with global variables. Static methods are global as well…so what. (This is no comment on OPs code…just general comments)

Yeah good point, maybe mentioning global variables was a bit of red herring when what I was really trying to raise was encapsulation and separation-of-concerns.