Inventory - how to handle items?

Question

I've been designing a game server, where a player has inventory, bank, and other features with items.

There are some definitions of what item can be in my game:

• Item is either stackable or not stackable.
• If item is stackable, item's amount can be up to 100 per stack.

So since each item can have an amount if it's stackable, my regular idea was having two arrays, int[] itemIds and int[] amount, where itemIds holds the IDs of the items, and amount holds the amount for item in that index.

I must keep the indexes the same, because when the client selects an item, it sends over the slot id he clicked on.

But then I thought, I could just do an Item object, with the tools in that class for that item, and have a private property isStackable and amount.

But then my mate said the following thing:

If you want to change some value of an Item object, you'll need to call a new construct and then get the old value then add the new value and then pass it to the construct. with primitives you can do that instant, which gives also performance positives

But then I thought of an idea, have a class named ItemStorage which will contain two arrays, items and amount like I described above:

public class ItemStorage {

    private int[] items;
    private int[] amount;

    public void setItems(int[] a, int[] b) {
        this.items = a;
        this.amount = b;
    }

    public int[] getItems() {
        return this.items;
    }

    public int[] getAmount() {
        return this.amount;
    }

}

I have a class called Inventory, it's my domain object for handling the inventory, it extends the interface Storage<ItemStorage>.

public interface Storage<T> {
    public T getStorage();
}

Now, since it implements it, the class must have the method getStorage() which will return ItemStorage, so to modify items, you do inventory.getStorage().getItems()[0] = 50;

But I am not sure about it, I am sure that List<Item> or Item[] will be much friendly, but I am not really sure why my mate dislikes it.

What do you think about my idea, and his opinion? Do you have another idea I can use for this?

How the items are handled in the game

The game contains over 25,000 items, each item is unique. The game uses each item as an id to represent it's uniqueness. The first item out of that item list, will have an id of 1.

Lets look at item 1445, It's a sword.

When the client wants to wear the item, he has to click on it in his inventory. This click will send a packet to the server, with the slot id he clicked in the inventory.

There's an example, similar to my inventory:

Let's base on that picture.

You see 16 slots in that picture, starting from 0 to 15. Player clicks on slot 4, and the client will pass the slot 4 to the server. The server will check through the inventory, what item is sitting on slot 4 in the array, it will see the amulet with id 555 for example (like in the picture).

Therefore I need slots to be supported in my design.

Answer 1

About your existing code

There's only two things I have to complain about in your current ItemStorage.

1. public void setItems(int[] a, int[] b) See those parameter names? a and b. They represent completely different things, they should have been named itemIds and amount.

2. Item(id)s in one array, and amount in another. What if each item would have enchantments? (which, for the sake of simplicity, also is an int for now) Would you add private int[] enchantments? Then what if you would add damage for items? Or other properties? It's a bad bad idea to store it like this. It's not all about performance, it's also about extensibility.

But then I thought, I could just do an Item object, with the tools in that class for that item, and have a private property isStackable and amount.

Yes, yes, YES!!

If you want to change some value of an Item object, you'll need to call a new construct and then get the old value then add the new value and then pass it to the construct. with primitives you can do that instant, which gives also performance positives

No, no, NO!!

If by "construct" you mean constructor, then no - use a setSomeValue(int newValue) method instead. If by "construct" you mean a set method like setSomeValue(int newValue), then sure, if you want to change some value you'd have to do item.setSomeValue(item.getSomeValue() + 3); but that's fine. This won't affect performance much. Java even use a Just in Time compiler to boost the performance of such calls.

If you want to you could even add a method like this to your Item class:

public void changeSomeValue(int change) {
    this.someValue += change;
}

This is especially useful for values you change often without caring much about the actual value at the moment, such as changing a player's HP. (I personally often call methods such as player.heal(4) or player.damage(6))

Designing an Inventory and Items

I think you should look at how the API of Bukkit (the Minecraft server) has been made. Bukkit have a class for ItemStack and also an interface for Inventory. You can probably find a lot of inspiration from Bukkit, although you probably won't even need half of it.

Here are some example classes that I think you could use:

public class ItemStorage {
    private Item[] items;
    private SomeObject owner; // Player, Chest, whatever... if you need it.
}

public enum ItemType {
    WEAPON(1, 1), SHIELD(2, 1), MONKEY(3, 4), MUG(4, 42), FISH(5, 64);
    // You never know what you might need in your game

    private final int id;
    private final int maxStackSize;
    private ItemType(int id, int maxStackSize) {
        this.id = id;
        this.maxStackSize = maxStackSize;
    }
}

public class Item {
    private ItemType type;
    private int amount;
}

If you want to somehow dynamically add items, then an enum won't work well for you, use an int in that case. Although I think an enum is good enough for now.

Answer 2

• Maybe you could rename ItemStorage to Slots, or PlayerInventory.

  public class PlayerInventory {
     private static final int MAX_NUM_ITEMS = 16;
     private List<Integer> itemIds;
     // map: item id -> count
     // (note that map: slot index -> count 
     //       would also be possible, but I prefer the alternative)
    private Map<Integer, Integer> counts; 
  
    // Setter must be careful about item count > 16.
    // Incrementing and decrementing item counts must watch out for: 
    //   1) when the count goes < 0, you must remove the item from the List
    //   2) uncountable items should not have a count, or an unmodifiable count of 1.
  

  I guess you also have a Map<Integer, Boolean> somewhere that tells you which items are countable.

  I am using a List instead of an array, since it will resize automatically and you can get the number of occupied slots with List.size(). I also assume that you want the items to always occupy the first slots (ie, you won't have the first two slots occupied than a gap and another item in slot four).

• I don't understand Storage<ItemStorage>. I guess you have some logic that is shared between the slots inventory and other types of storage, but we have no information about that. It does not seem useful to have such an overclass unless the member types share some commonality: Storage<T extends XXXX>.

EDIT:
I want to expand on one of my comment (with some input from @Simon). Your whole design should probably be rethought. You probably have a database with item name, item id, description, stackability, etc. It seems that when you read this database in memory, you split the info all over, mostly in arrays where the index is a reference to the item id. It would be much cleaner to just transform each line from the database in an instance of a class that mirrors the info in the database.

Look into JPA, or some other object relational mapping (ORM) framework; you just write your Item class with its members and annotate the class with @Entity and fetching/persisting from the database is automatic. [EDIT: I realized that you wrote that the stackability properties are stored in a config file -- and maybe all your data is in config files --, but you should probably switch to a database, especially since you have 25,000 items.]

You probably gain very little performance by spreading the info in various arrays. And it looks messy. Also, if you ever decide to definitely remove item #344 "Crystal Magic Sword" from the game, you are in trouble.

You can transfer the whole database of items into a variable Map<Integer, Item> items (item id -> item). The item id is the map key and is also a member of the associated Item; it's a common pattern. Note that this map should be defined when you read the database and never be modified later, so you should define it as final and wrap it in a Collections.unmodifiableMap(...).

And my solution relating to each player remains the same: each player as a member variable Map<Integer, Integer> counts (item id -> count). Or you could instead have Map<Item, Integer> counts.

Also, we can simplify

public class PlayerInventory {
   private List<Item> items;
   private Map<Item, Integer> counts;
   ...

to

public class PlayerInventory {
   private LinkedHashMap<Item, Integer> items;  
   ...

The keys in a LinkedHashMap conserve their ordering, so LinkedHashMap.keys() can replace the member List<Item>. So PlayerInventory is just a LinkedHashMap and it might look like we should not define that class at all and use the LinkedHashMap directly instead. However, you still need that class since you need to define specialized methods to not add more than 16 items; to only change the count on items which are stackable; and to remove stackable objects when their count drops to zero.

---

I'm sorry if my code review got a bit off track and ended up being a review of your whole system. You probably can't implement everything I mentioned since it would be an awful lot of work. Nonetheless, my review will be accessible to others and it might be useful to them. And maybe you'll do some major refactoring at some point.