How to simplify existing Inventory System?

Yeah I try to rewrite my Inventory :cranky:
So far it´s working and passing all Unittests but the more Testcases I find the more I have to rewrite, not the logic part, but the overall Dependencies.

I have an Item class and one Itemstack, which holds one item and it´s amount.
The Inventory is an List of InventorySlot and this InventorySlot holds an Item or an ItemStack.
Especially that InventorySlot can hold both seems to be problematic. Or that now Item needs to have an currentAmount Method only to be able to have it for the ItemStack.
Or ItemStack now has an isEquippable Method etc…

Furthermore I don´t knew yet if I will distinguish different Materials with an enum, with interface or even with ECS Components.

Any idea how to redo my class dependencies?

[spoiler]


public class MyItem extends AbstractItem {

    public MyItem(String name, int itemId, int maxStackCount) {
        super(name, itemId, maxStackCount);
    }

    public MyItem(String name, int itemId) {
        super(name, itemId);
    }

public class InventorySlot<T extends IItem> {
    public int id;
    private T item;

    public InventorySlot(int id) {
        this.id = id;
    }
    boolean isOccupied() {
        return item != null;
    }
    public void add(T item) {
        this.item = item;
    }
    public T getItem() {
        return this.item;
    }

public abstract class AbstractItem extends AGameObject implements IItem {
    private String name;
    private int itemId,maxStack;
    private boolean isStackable;
    
    AbstractItem(String name, int itemId, int maxStackCount) {
        super();
        this.name = name;
        this.itemId = itemId;
        this.maxStack = maxStackCount;
        if (maxStackCount > 1) this.isStackable = true;
    }

    AbstractItem(String name, int itemId) {
        this(name, itemId, 1);
    }

    //no need for this
    public int getCurrentStackCount() {
        return 1;
    }

[/spoiler]


public class ItemStack implements IItem {

    long stackId;
    IItem item;
    int currentCount;

    public ItemStack(IItem item) {
        this.item = item;
        this.currentCount = 1;
    }

    @Override
    public void removeAmount(int count) {
        this.remove(count);
    }

    boolean canAdd(IItem item) {
        if (item.equals(item)) {
            return true;
        }
        return false;
    }

    boolean canAdd(ItemStack stack) {
        if (item.equals(stack.item)) {
            return true;
        }
        return false;
    }

    public int getCurrentStackCount() {
        return this.currentCount;
    }

    public boolean add(IItem newItem) {
        //item which wants to be added is not the same as in the stack
        if (!canAdd(item)) return false;

        //this stack is already at maximum Stackvalue
        if (this.currentCount == item.maxStackCount()) return false;
        this.currentCount++;
        return true;
    }

    public ItemStack add(ItemStack stack, int transferCount) {
        return null;
    }

    public ItemStack add(ItemStack stack) {
        return this.add(stack, stack.currentCount);
    }

    boolean remove(int count) {
        if (this.currentCount - count >= 0) {
            this.currentCount -= count;
            return true;
        } else {
            return false;
        }
        //TODO item less or equal than zero
    }

    public long getId() {
        return stackId;
    }

    boolean isEmpty() {
        if (this.currentCount <= 0) return true;
        return false;
    }
}

public class Inventory<T extends IItem> {
    Set<InventorySlot> slots = new HashSet<>();
    private int maxSlots;

    public Inventory() {
        this(64, 8, 8);
    }

    public Inventory(int rows, int cols) {
        this.inventoryRows = rows;
        this.inventoryCols = cols;
    }

    public Inventory(int size, int maxRows, int maxCols) {
        maxSlots = size;
        this.maxCols = maxCols;
        this.maxRows = maxRows;
    }

    public boolean add(T item) {  
        InventorySlot freeSlot = findFreeSlot();
        if (freeSlot == null) return false;
        freeSlot.add(item);

        return true;
    }


    /**
     * removes an item ENTIRELY from the Inventory
     *
     * @param item
     */
    public void remove(T item) {
        InventorySlot slot = this.slots.stream().filter(x -> x.getItem().getItemId() == item.getItemId()).findFirst().get();
        slot.add(null);
    }

    /**
     * removes given amount of an item form the Inventory, only works if amount >1
     *
     * @param item
     * @param amount
     */
    public void remove(T item, int amount) {
        InventorySlot slot = this.slots.stream().filter(x -> x.getItem().getItemId() == item.getItemId()).findFirst().orElse(null);
        slot.getItem().removeAmount(amount);
    }

    public InventorySlot getSlotByItem(IItem item) {
        return this.slots.stream().filter(x -> x.getItem().getItemId() == item.getItemId()).findFirst().orElse(null);
    }

    public InventorySlot getSlotAt(int place) {
        return this.slots.stream().filter(x -> x.id == place).findAny().orElse(null);
    }

    private InventorySlot findFreeSlot() {
        InventorySlot freeSlot = this.slots.stream().filter(x -> !x.isOccupied()).findFirst().orElse(null);
        return freeSlot;
    }

    public int getUsedSlots() {
        return (int) this.slots.stream().filter(x -> x.isOccupied()).count();
    }

    public Set<InventorySlot> getItems() {
        return this.slots;
    }

Edit: deleted some unimportant Methods and getter/setter.
btw: Is the Spoiler tag not working?

Edit2: reduced unnecessary Code…

Why do you have abstraction here that goes above what there should be.

An inventory can be as simple as an unordered table of all necessary data.

So I would drop abstraction. I would create a list of all items and load it in. (Yeah don’t create 100 item classes please)

1 Dagger
2 Gold
3 Princec’s Love For Java
4 Bucket

Then add these to a table. Call it your ‘lookup table’. Then you can refer to it like Inventory.isItemWeapon(player_invy.get(0));

@Hydroque has a good solution to this issue.

An aside about software development:

Abstraction is good when dealing with complicated systems and abstract ideas. For something as simple as an inventory, there is no need to abstract this concept further using abstract classes. The same is true for “Item” and “ItemStack”. If desired, you can use interfaces to provide a clear and explicit definition of these objects. In the code that you posted, there is a lot of unnecessary abstraction. (Your example actually makes the concepts more difficult to understand)

Combining Hydroque’s approach with yours:

All you need are three classes: Item, ItemStack, and Inventory
(giving Inventory its own class will help with abstraction down the road)

Item should contain the basics of what an Item is:
an ID, Title, Description, etc.

There are multiple ways that you could conceptualize your list of Items…

  1. Reference a static list of items using aliasing to create instances of particular items (I think Minecraft likes to do things this way, if I remember correctly)
  2. Load items from something similar to JSON into a table (Probably only want to mess with this if you’re planning a huge project)
  3. Enums (blegh)
  4. Flyweight or Factory design patterns
  5. many many more…

ItemStack should merely be a container for Items:
It should contain what Item it is referring to, how many of that Item, and possibly a maximum stack size for that Item.

Inventory should be a structure that allows for indexing such as a 2-D table.
This structure should only contain ItemStacks, no Items.
It should provide methods to add ItemStacks and remove ItemStacks only.

If you want something that is easily sortable, you could use an ArrayList with a Comparator.
(If this interests you, I could explain this idea further)

When designing systems in the future, try to keep “KISS” in mind. (Keep It Simple, Stupid!). In general, the simpler a solution is, the more elegant it will be and the easier it will be to work with.

Cheers,

-QuicK