Structuring a storage system for a game

Question

I am working on a multiplayer game, where a player has many storages to store items in.

Storage

Storage is the system that allows the user to deposit items, and take them back. Example of storage types:

• Inventory: Same as WoW inventory, you can add items, drop items, move items from slot to slot.
• Bank: Bank is what you can call, a super-inventory, which has around 300 items limit per tab Bank can have maximum of 10 tabs which will store items. total items allowed in the bank is 3000 (10 tabs multiplied by 300 items per tab). How ever, in the bank, all items are stackable.
• Equipment: Equipment is the same as Inventory, just with sorted slots, i.e Helm, Body, Legs and there are total of 8 slots.
• Chests and other: Same as Bank just without tabs.

An item in-game

Each item has it's own item id. Only certain items can be stackable, unless the storage type is Bank or chest.

I have created a system which will make it easier for me to manage storages, and there's the abstract class, that each storage type class will extend it:

public abstract class PlayerItemStorage {

    private int size;

    /**
     * The items
     */
    private PlayerItem[] items;

    /**
     * Sometimes, by default, the storage items are id + 1, due to
     * the client's interfaces. No exact explanation on it, but the only
     * feature that doesn't do that is equipment and possible BoB
     */
    private boolean increasedItem = true;

    public PlayerItemStorage(int storageSize) {
        this.items = new PlayerItem[storageSize];
        this.size = storageSize;
    }

    public PlayerItemStorage(PlayerItem[] items) {
        this.items  = items;
        this.size = items.length;
    }

    /**
     * Resets the storage
     */
    public void resetStorage() {
        this.items = new PlayerItem[this.size];
    }

    /**
     * Gets the item list
     * @return PlayerItem list
     */
    public PlayerItem[] getItems() {
        return this.items;
    }

    /**
     * Counts how many items there are with the same id.
     * @param id Item id
     * @return count
     */
    public int count(int id) {
        int count = 0;
        for (PlayerItem item : this.items) {
            if (this.getId(item) == id) {
                count++;
            }
        }
        return count;
    }

    /**
     * Counts how many items there are in the inventory
     * not including amount, just the item objects themself.
     * @return amount
     */
    public int countItems() {
        int count = 0;
        for (PlayerItem item : this.items) {
            if (this.getId(item) > 0 && item.getAmount() > 0) {
                count++;
            }
        }
        return count;       
    }

    /**
     * Gets the length of the storage
     * @return
     */
    public int getLength() {
        return this.items.length;
    }

    /**
     * Checks how many free slots the storage has
     * @return
     */
    public int freeSlots() {
        int slots = 0;
        for (PlayerItem item : this.items) {
            if (this.getId(item) <= 0) {
                slots++;
            }
        }       
        return slots;
    }

    /**
     * Finds an item id in the earliest slot.
     * @param id Item ID
     * @return Slot id
     */
    public int find(int id) {
        for (int i = 0; i < this.items.length; i++) {
            PlayerItem item = this.items[i];
            if ( this.getId(item) == id && item.getAmount() > 0) {
                return i;
            }
        }
        return -1;      
    }

    /**
     * Gets the earliest free slot
     * @return slot id
     */
    public int getFreeSlot() {
        for (int i = 0; i < this.items.length; i++) {
            if (this.getId(i) <= 0) {
                return i;
            }
        }
        return -1;
    }

    /**
     * Checks if item is existing in the given slot, with the given amount and 
     * item id.
     * @param id Item ID
     * @param amount Item Amount
     * @param slot Slot id
     * @return boolean
     */
    public boolean containsInSlot(int id, int amount, int slot) {
        int found = 0;
        if (this.items[slot].getAlpha() == id) {
            for (int i = 0; i < this.items.length; i++) {
                PlayerItem item = this.items[i];
                if (this.getId(item) == id) {
                    if (item.getAmount() >= amount) {
                        return true;
                    } else {
                        found++;
                    }
                }
            }
            if (found >= amount) {
                return true;
            }
            return false;
        }
        return false;
    }

    /**
     * Checks if item exists in the storage
     */
    public boolean contains(int id) {
        for (PlayerItem item : this.items) {
            if (this.getId(item) == id) {
                return true;
            }
        }
        return false;
    }

    /**
     * {@link #increasedItem}
     * @param i Index
     * @return real item id
     */
    private int getId(int i) {
        if (this.increasedItem) {
            return this.items[i].getAlpha();
        }
        return this.items[i].getId();
    }

    /**
     * {@link #increasedItem}
     * @param item PlayerItem object
     * @return real item id
     */
    private int getId(PlayerItem item) {
        if (this.increasedItem) {
            return item.getAlpha();
        }
        return item.getId();    
    }

    public abstract boolean add(int id, int amount);
    public abstract boolean remove(int id, int amount, int fromSlot);
}

I also have a container class which is in charge to control all storages:

/**
 * Container class for all storages of a player
 * @author Ben
 */
public class PlayerStorageContainer {

    /**
     * Player object (Client extends Player extends Entity)
     */
    public Player player;

    /**
     * boolCatach: If enabled, when removing/adding item to ALL of the storages,
     * it will check if the action returned false, if yes it will stop right away and
     * return false.
     */
    private boolean boolCatch;

    /**
     * All of the storages the player will contain
     */
    private Map<String, PlayerItemStorage> storages = 
            new HashMap<String, PlayerItemStorage>();

    public PlayerStorageContainer(Player p) {
        this.player = p;
        this.initializePlayerStorages();
    }

    /**
     * Initializes the existing storage for player.
     */
    private void initializePlayerStorages() {
        this.storages.put("inventory", new Inventory(this.player));
    }

    /**
     * Gets the required PlayerItemStorage implementation
     * @param name Name of the implementing object
     * @return The object that extends PlayerItemStorage
     */
    public PlayerItemStorage getStorage(String name) {
        return this.storages.get(name);
    }

    /**
     * Removes an item from ALL storages
     * @param id        The Item ID
     * @param amount    The amount of the item
     * @return Returns a boolean if action was successful without any falses in the
     * middle of the execution, will return always true if {!boolCatch}
     */
    public boolean remove(int id, int amount) {
        for (PlayerItemStorage storage : this.storages.values()) {
            if (!storage.remove(id, amount) && this.boolCatch) {
                return false;
            }
        }
        return true;
    }

    /**
     * Adds an item to ALL storages
     * @param id        The Item ID
     * @param amount    The amount of the item
     * @return Returns a boolean if action was successful without any falses in the
     * middle of the execution, will return always true if {!boolCatch}
     */
    public boolean add(int id, int amount) {
        for (PlayerItemStorage storage : this.storages.values()) {
            if (!storage.add(id, amount) && this.boolCatch) {
                return false;
            }
        }
        return true;
    }

    /**
     * Resets all storages
     */
    public void reset() {
        for (PlayerItemStorage storage : this.storages.values()) {
            storage.resetStorage();
        }
    }

    /**
     * Toogles {@link PlayerItemContainer#boolCatch} for errors
     * @param b
     */
    public void setErrorCatch(boolean b) {
        this.boolCatch = b;
    }
}

The problem

This system works fine for inventory, and equipment - however, this will be a problem and cause some big algorithm code duplicating for the bank type.

Since bank type storage can have multiple tabs up to 10, we will either need to create class named Tab and make it implement PlayerItemStorage, and put the same algorithm over the two classes (Bank and Tab) or have one method in Bank, and then have a variable named "focusedTab" which will hold the selected tab id, and make the add method accept 3 arguements (itemId, amount, tabId).

But making it accept 3 arguments will make a problem, I'll have to either ignore the abstract method add, and make own method for it, or create 2 abstract adds method, one with 2 parameters, one with 3 which I find too messy.

Did I design this whole thing incorrectly that I am having these problems? How can I design a proper structure for this whole storage feature so it works with all kind of storages I can think of?

If you cannot understand the problem, you might find my code review thread helpful

Background about how the server handles items

When the client clicks on any item, in any game interface that supports item storage, it sends the slot id the player clicked on, due to security reasons. The server uses the slot id as the index of the PlayerItem[] array to get the information about the item.

Answer 1

I think you've confused having an interface for your program to call to add/remove items for a particular player and being able to manage all individual cases. The interface to the rest of your program should remain simple. By simple I mean it should be single-purpose and the only methods public will be those that will be called.

As I understand it, it is PlayerItemStorage that handles the logic about fitting and physical layout of those items. This sort of complication should remain within PlayerItemStorage. For the callers of PlayerItemStorage, they should only care about what items are present, if an item has been removed or added. When this happens, through a PlayerItemStorageChangeHandler interface, any instance in your program that wants to know about this event can register. Your PlayerStorageContainer would use this as a means to get this information and act consequently.

Any validation about whether or not they can fit it in storage should be entirely managed by PlayerItemStorage. Notice that I said that whether or not an item has been removed or added should be handled by PlayerItemStorage. Do not be tempted to put the logic in PlayerStorageContainer since it should not know whether or not it is possible.

Granted, I still think adding and removing items from PlayerStorageContainer should be possible, however you should allow the possibility of failure. Anything that you would want to happen as a consequence of adding it to PlayerItemStorage should be put in the same place as your PlayerItemStorageChangeHandler listener for new adds/removes (again, avoiding to duplicate your code). Also, I presume that the main interface to be called when a player adds/removes items to his storage should be PlayerItemStorage and not PlayerStorageContainer for most operations, however I suppose if you say wanted to add a gift item to a player's storage, it would be preferable not to care which storage it is (just throw it anywhere where there's room).

Once you've established this relationship, you'll see that a bank concept with tabs is nothing more than a collection of PlayerItemStorage. I would therefore create a special type of PlayerItemStorage called PlayerBankItemStorage. It would override the same operations called by PlayerStorageContainer, but it could also maintain open tab state. Try to keep the logic simple though. You should forward most calls to PlayerItemStorage. PlayerBankItemStorage would listen to PlayerItemStorageChangeHandler and then throw the event itself (if it should handle events at least, otherwise adding a new listener should simply forward to each individual PlayerItemStorage.

The key thing to remember is to avoid duplicating logic. Keep class roles clear in your mind, and you'll find that it is easier to delegate tasks to your classes and keep logic in one place only.