I was wondering if it's normal/efficient to have this many static collections in my state based game? I started doing it for ease of access but it looks sloppy to me and I was just wondering what conventions other people use to maintain references to game objects?

public class DungeonDigger extends StateBasedGame {
    public static NetworkPlayer myCharacter;
    public static Server SERVER = new Server();
    public static Client CLIENT = new Client(); 
    public static int CHOSEN_GAME_STATE;
    public static final int MAINMENU = 0;
    public static final int LOBBY = 3;
    public static final int SINGLEPLAYERDUNGEON = 1;
    public static final int MULTIPLAYERDUNGEON = 2;
    public static String ACCOUNT_NAME;
    public static String IP_CONNECT;
    public static int MAX_MESSAGE_LENGTH = 50;
    public static ConnectionState STATE;
    public static HashMap<String, Image> IMAGES = new HashMap<String, Image>();
    public static ArrayList<TextPacket> TEXTS = new ArrayList<TextPacket>();
    public static LinkedList<ChatPacket> CHATS = new LinkedList<ChatPacket>();
    public static HashMap<String, NetworkPlayer> CHARACTERBANK = new HashMap<String, NetworkPlayer>();
    public static HashSet<String> ACTIVESESSIONNAMES = new HashSet<String>();
    public static HashMap<Integer, String> KEY_BINDINGS = new HashMap<Integer, String>();
    public static HashMap<String, String> SLOT_BINDINGS = new HashMap<String, String>();
    public static HashMap<String, Ability> ABILITY_TEMPLATES = new HashMap<String, Ability>();
    public static HashMap<String, Mob> MOB_TEMPLATES = new HashMap<String, Mob>();
    public static Vector<Ability> ACTIVE_ABILITIES = new Vector<>();
    public static AbilityFactory ABILITY_FACTORY = new AbilityFactory();
    public static MobFactory MOB_FACTORY = new MobFactory();

    public DungeonDigger(String title) {

    // Start game
    public static void main(String[] args) {
        try {
            System.setProperty("org.lwjgl.librarypath", new File(System.getProperty("user.dir"), "natives").getAbsolutePath());
            System.setProperty("net.java.games.input.librarypath", System.getProperty("org.lwjgl.librarypath"));
            AppGameContainer app = new AppGameContainer(new DungeonDigger("Dungeon Digger"));
            app.setDisplayMode(640, 640, false);
        } catch (SlickException e) {

Overall, separate your code, make it modular. Store state where it needs to be stored, but keep it separate, and expose the minimum amount of functionality that you have to.

I would highly recommend that you read the book Effective Java, and then read it again. Feedback below, separate thoughts separated by . . .

. . .

First, don't use ints for storing flags, use enums.

public static int CHOSEN_GAME_STATE;
public static final int MAINMENU = 0;
public static final int LOBBY = 3;
public static final int SINGLEPLAYERDUNGEON = 1;
public static final int MULTIPLAYERDUNGEON = 2;

What does (MAINMENU - LOBBY * SINGLEPLAYERDUNGEON / MULTIPLAYERDUNGEON) mean? It's nonsense, but it's a valid expression in your API.

Instead, it would be much better annotated as:

enum GameState {
private static GameState sCurrentGameState = GameState.MAIN_MENU;

. . .


public static HashMap<String, Image> IMAGES = new HashMap<String, Image>();

You have no control over who modifies your images. Instead I'd make your images stored and loaded via their own structure, something like:

class ImageCache {
    private HashMap<String, Image> mImages = new HashMap<String, Image>();

    public Image getImage(String imageName) {
        Image result = mImages.get(imageName); //returns null if image not present.
        if (result == null) {
             result = Image.loadFromFile(imageName);
             mImages.put(imageName, result);
        return result;

    public boolean imageCached(String imageName) {
        return sImages.contains(imageName);

This will give you a lot of things. First, let's say that you have too many images to store in memory, now you can easily implement an LRU cache or something to load on demand. And better yet, so long as getImage and imageCached work as they did before, this will all be hidden from the people using the ImageCache.

. . .

Instead of instantiating factories, I would recommend that you use static factories. So instead of (assuming MobFactory returns Mob objects):

public static MobFactory MOB_FACTORY = new MobFactory();

You would have something like:

class Mob {
    //Private ctor means that nobody can instantiate this directly.
    private Mob(String name, int hp, int mp) { ... }

    //Create the Ability, give it to the entity
    public static Mob create(String name, int hp, int mp) {
        assert hp > 0 : "Tried to create a mob with " + hp + " HP!";
        assert mp >= 0 : "Tried to create a mob with " + mp + " MP!";
        Mob mob = new Mob(name, hp, mp);
        //optionally manipulate the mob further
        return mob;

You could be even more clever, and roll your templates into this, so that they are entirely hidden from the user.

. . .

Don't use Vector, it is a concurrent data structure. Since it doesn't look like you are using concurrency, stick with ArrayList.

. . .

I find it's useful to have a static settings class for things like MAX_MESSAGE_LENGTH. That way it's easy to figure out where constants are defined, and to save/load them from disk. . . .

  • \$\begingroup\$ I'll be taking a great many of your suggestions into my code. Thank you very much for the feedback. \$\endgroup\$ – Sarophym Jan 26 '12 at 3:22
  • \$\begingroup\$ @Sarophym: No problem! Post your next iteration, I can rip that one apart too :P \$\endgroup\$ – mindvirus Jan 31 '12 at 3:13

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.