3

I have a java game I'm writing where I need a master database of unit archetypes. The database is just a class containing a HashMap that stores a couple of dozen class instances containing individual unit types' stats. When the game spawns a new unit, it copies the unit out of the database, using the unit's name to locate it in the HashMap. This database gets built once, when the program starts, and doesn't change. I am also not extending or modifying any of the classes stored in the HashMap. It's meant to be a read-only reference for use by the game systems.

I have several other classes, basically armies, that contain a number of units. When an army gets a unit, it copies the unit's info out of the database. I have three ways of providing the army classes with a way to read the master database. I'd like your opinion as to which is the best solution for creating a simple, readable code that won't create weird bugs.

I've included some simple code to illustrate the different approaches (sorry if I missed a semicolon or something, I put my examples together quickly and I'm still learning java).

Method 1) I create the database as a class instance, and every time I create a new army or call an army method that adds a unit to the army, I pass the army or method a reference to the database instance as a parameter. This should be pretty easy to visualize.

Method 2) I create the database as a class instance. In the army class, I have a static reference to the database instance that gets set once by a static method in the army class. After the database is populated with data, the static method is called and sets the static reference to point to the database. Thereafter, the army class will always be able to pull information from the database simply by referencing the static variable.

class database
{
    // HashMap of unit archetypes
    private HashMap<String, Unit> unitLibrary = new HashMap<String, Unit>();

    // method for storing units in HashMap
    void storeUnits(String fileName)
    {
        // load unit stats from file
        // add new Unit instances to unitLibrary
    }

    // getter method
    Unit getUnit(String unitName)
    {
        return unitLibrary.get(unitName);
    }
}

class Army
{
    // variables
    private static Database masterDatabase;
    private static boolean databaseSet = false;
    ArrayList<Unit> armyUnits = new ArrayList<Unit>();

    // method for creating static database reference
    void setReference(Database d)
    {
        // set static reference to main database if not previously set
        if (!databaseSet)
        {
            masterDatabase = d;
            databaseSet = true;
        }
    }

    // add unit to army
    void addUnit(String unitName)
    {
        armyUnits.add(masterDatabase.getUnit(unitName);
    }   
}

public class CodeTest
{
    public static void main(String[] args)
    {
        // create master database
        Database masterDatabase = new Database();
        masterDatabase.storeUnits("FileOfUnits.game");

        // set static reference in army class to point to master database
        Army.setReference(masterDatabase);

        // Create army
        Army army1 = new Army();
        army1.addUnit("Soldier");
    }
}

Method 3) I create the HashMap in the database class as static, and use a static method to populate it with data. The getter methods in the database class are also made static. Now there is no passing of references at all, because every time an army class instance needs to pull from the database, it just runs Database.getUnit().

class database
{
    // HashMap of unit archetypes
    private static HashMap<String, Unit> unitLibrary = new HashMap<String, Unit>();

    // method for storing units in HashMap
    static void storeUnits(String fileName)
    {
        // load unit stats from file
        // add new Unit instances to unitLibrary
    }

    // getter method
    static Unit getUnit(String unitName)
    {
        return unitLibrary.get(unitName);
    }
}

class Army
{
    ArrayList<Unit> armyUnits = new ArrayList<Unit>();

    // add unit to army
    void addUnit(String unitName)
    {
        armyUnits.add(Database.getUnit(unitName);
    }   
}

public class CodeTest
{
    public static void main(String[] args)
    {
        // prepare master database
        Database.storeUnits();

        // create army
        Army army2 = new army2();
        army2.add("Soldier");
    }
}

I've tried all three methods, and they all work, but the code is still in its infancy and I don't want to encounter tricky bugs down the road because of my early architecture. I understand that it isn't encapsulated in the traditional object-oriented manner, but Method 3 actually seems the cleanest to me because the database gets created in the background and never has be piped through a bunch of different method calls to be accessed. The code also has fewer lines and no tricks like the one in method 2.

Any suggestions or experience with something like this? I've done a lot with python, but I've only recently started to learn java, and still trying to get used to the encapsulation restrictions.

CMB
  • 133
  • 4
  • First of all: program against interfaces. Meaning: use List/Map as your types, and only worry about specific implementation classes when instantiating them. In other words: you only use HashMap/ArrayList after a `new`. Also learn about the diamond operator. Since Java 7, there is no need to repeat the generic arguments like you are doing here. Simple go `List armies = new ArrayList<>()`! – GhostCat Oct 02 '18 at 06:25
  • @vmrvictor So create a singleton and =ond – CMB Oct 02 '18 at 06:48
  • @vmrvictor So create a singleton instance in main and load it with data, then create another instance when the army constructor runs? Each new instance in an army points to the first one that was created in main? – CMB Oct 02 '18 at 06:51

3 Answers3

2

First, take into account that HashMap is not thread-safe. You cannot add new units in the database while clients are requesting references. Second, I'd have a Database interface and a DatabaseImpl class. Then inject an instance of DatabaseImpl via Unit constructor or setter. The main reason for this is testability aa you can easily mock the Database. In terms of memory management static objects will be automatically instantiated first on class load, but the garbage collector will put the Database in an old genetation after a few time, so I don't think it matters much in terms of performance.

Serg M Ten
  • 5,278
  • 4
  • 19
  • 37
2

Try to follow SOLID principles, from your code focus on Dependency Inversion Principle: Try to depend on abstractions (for example interface Database) and not your current implementation of Database.

I suppose that you should have some enums as unit type for Unit, I suppose you have different kind of Units. I suppose Unit should be abstract or even an interface because always you should have an specific unit (soldier, tank, ...), each kind of Unit should implement some kind of interface with actions (methods) as move, attack. Is not the same to move a soldier than to move a tank.

FileOfUnits.game as other configuration values that you will have I would use a configuration.properties file

Do not forget to test, TDD will do your classes easier to test and you will understand what you want to do before than implementing It.

Use some Control Version and commit/push with useful comments, will be easier to see why you have done some changes, in some days you will not remember why you have written these lines.

I recommend you to read and check in the following order the books: Effective Java and Clean Code

vmrvictor
  • 607
  • 6
  • 24
  • @Michael happy to get your feedback have removed this line as seems you are right, thanks for your hint – vmrvictor Oct 02 '18 at 08:03
  • 1
    Nice. I think that was spoiling an otherwise good answer. [Further reading](https://stackoverflow.com/questions/137975/what-is-so-bad-about-singletons), if you're interested – Michael Oct 02 '18 at 08:07
2

Method 1 is the best.

but Method 3 actually seems the cleanest to me because the database gets created in the background and never has be piped through a bunch of different method calls to be accessed

This is seems to be the crux of the issue to me. You're trying to solve dependency injection with static variables and it's a terrible idea. Frameworks like Spring can save you the trouble of passing objects down the chain with features such as autowiring if you think that that's a problem.

What if, in your second example, I create an Army and forget to set the database? I have an object that's effectively in a half-created state. Objects should never be in such a state. If you'd required it be passed to the constructor then you would have avoided this potential bug.

There's a maintenance problem with your second and third examples, as well, in that they restrict you to a single instance when you have no idea how your requirements might change. How do you know that two different armies will always share a database? What if in the future you want a ModernUSArmy and an AncientMacedonianArmy - do you know for certain that they will share the same archetypes?

Michael
  • 34,340
  • 9
  • 58
  • 100
  • The only excuse I can think of to not do proper DI using Spring or Guice is performance, but even then, having some sort of "context" or service provider class that gets passed in, that the objects can get what they need is cleaner then static variables. But most performance issues can be avoided by using factories where it counts anyway. – Ryan The Leach Oct 02 '18 at 08:10