HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Java Slick StateBasedGame managing resources across states

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
statebasedgamemanagingstatesjavaacrossresourcesslick

Problem

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 IMAGES = new HashMap();
public static ArrayList TEXTS = new ArrayList();
public static LinkedList CHATS = new LinkedList();
public static HashMap CHARACTERBANK = new HashMap();
public static HashSet ACTIVESESSIONNAMES = new HashSet();
public static HashMap KEY_BINDINGS = new HashMap();
public static HashMap SLOT_BINDINGS = new HashMap();
public static HashMap ABILITY_TEMPLATES = new HashMap();
public static HashMap MOB_TEMPLATES = new HashMap();
public static Vector ACTIVE_ABILITIES = new Vector<>();
public static AbilityFactory ABILITY_FACTORY = new AbilityFactory();
public static MobFactory MOB_FACTORY = new MobFactory();

public DungeonDigger(String title) {
super(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 AppGameContaine

Solution

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 {
    MAIN_MENU, LOBBY, SINGLEPLAYER_DUNGEON, MULTIPLAYER_DUNGEON
}
private static GameState sCurrentGameState = GameState.MAIN_MENU;


. . .

With

public static HashMap IMAGES = new HashMap();


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 mImages = new HashMap();

    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
        mob.giveAbility(Ability.DANCE);
        //...
        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.
. . .

Code Snippets

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;
enum GameState {
    MAIN_MENU, LOBBY, SINGLEPLAYER_DUNGEON, MULTIPLAYER_DUNGEON
}
private static GameState sCurrentGameState = GameState.MAIN_MENU;
public static HashMap<String, Image> IMAGES = new HashMap<String, Image>();
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);
    }
}
public static MobFactory MOB_FACTORY = new MobFactory();

Context

StackExchange Code Review Q#8297, answer score: 6

Revisions (0)

No revisions yet.