Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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:

img

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.

share|improve this question
    
    
    
@Jamal Is there a way to cancel bounty? Didn't except to do a bounty for this question, but for the updated one. –  user3123545 Jul 1 at 16:21
    
Moderators (such as myself) have the power to cancel a bounty before it gets awarded. Since you weren't already aware of the review policy (hopefully you do now), I'll go ahead and cancel it. Keep in mind that this is only done at a moderator's discretion. –  Jamal Jul 1 at 16:25
    
@Jamal Alight, thanks a lot. –  user3123545 Jul 1 at 16:28
add comment

2 Answers 2

up vote 16 down vote accepted

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.

share|improve this answer
    
The thing is, I can't use ItemType mate, there are over 25,000 items in-game with different categories. So it's only by item id. –  user3123545 Jun 28 at 20:33
    
@user3123545 int it is then! –  Simon André Forsberg Jun 28 at 20:40
    
Edited my question. –  user3123545 Jun 28 at 20:46
    
Bukkit IIRC uses a lot of hashmaps. My friend said why use hashmap when it takes 200 nanoseconds to get a value by key using it, where you can make a suitable class with raw arrays for your inventory. –  user3123545 Jun 28 at 20:48
    
@user3123545 Yes, Bukkit uses a lot of HashMaps, but not for the inventory contents itself. Bukkit uses ItemStack[] getContents(); for the inventory, just like I use Item[] items in my example code above. I agree that it's better to use an array than a HashMap, especially if the HashMap would use Integers as a key. –  Simon André Forsberg Jun 28 at 20:54
show 1 more comment
  • 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.

share|improve this answer
    
It's not a map, I have a static which contains a array of booleans for each item id, it loads the booleans from a config file. So Items.isStackable(id) –  user3123545 Jun 28 at 21:28
    
OK. Same as a map. –  toto2 Jun 28 at 21:29
    
@user3123545 You might want to consider sharing some more code for review (in another question, preferably). I don't think that sounds optimal. It would be interesting to understand a bit more about how you store the information about all your item ids. –  Simon André Forsberg Jun 28 at 22:08
    
@Simon I'm not sure what you are referring to, but using a boolean[] as a Map<Integer, Boolean> seems fine to me. Maybe just slightly less readable. However, I agree that if I were to write the whole code base from scratch, I might do things differently. –  toto2 Jun 28 at 22:21
1  
I assume there is some database where each line has an item id number, item name, description, stackability, etc. When the database is read, it would be much cleaner if each line was transformed to some object containing all that info instead of spreading that database information in various arrays all over the place. But using arrays where the index represents the item id might offer some performance advantages. We would probably need a lot more information to figure out what is best. –  toto2 Jun 28 at 22:56
show 4 more comments

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.