1

In my multiplayer game I keep track of each player's inventory. I have a class for each inventory action that extends the abstract class InventoryItem. I then polymorphically call use() on the InventoryItem abstract class in my Inventory class which is the public API for the higher-level packages.

class Inventory {
    private final Map<Class<? extends InventoryItem>, Integer> itemBag;
    // ...
    
    public boolean useItem(InventoryItem item) {
        int itemCount = countItem(item.getClass());
        if (itemCount < 1)
            return false;

        boolean success = item.use(player, gameMap);
        if (success)
            itemBag.put(item.getClass(), itemCount - 1);
        return success;
    }

    public void addItem(Class<? extends InventoryItem> ability) {
        itemBag.put(ability,countItem(ability) + 1);
    }

    public int countItem(Class<? extends InventoryItem> item) {
        return itemBag.getOrDefault(item, 0);
    }
    // ...
}

public abstract class InventoryItem {
    abstract boolean use(Player player, GameMap game);
}

// Sample inventory item and power-up subclass
// Powerup extends InventoryItem
public class ExpandAirplaneCommand extends Powerup {
    private final Airplane airplane;

    public ExpandAirplaneCommand(Airplane plane) {
        airplane = plane;
    }

    @Override
    boolean use(Player player, GameMap game) {
        airplane.expand();
        return true;
    }
}

The instances of these subclasses are specific commands with specific parameters, but the subclasses themselves represent a type of inventory item, and this is what I need to count in my itemBag. Thus I used the Class<? extends InventoryItem.

This design has worked extremely well for simply using the items and counting any specific item. But now there's some functionality that I need to add to it. Some of these inventory items extend a Powerup abstract class (couldn't be an interface because its methods need to be package-private). A mechanic of the game involves counting the number of powerups. This means I need to check the itemBag for each InventoryItem subclass for the corresponding count of that type.

So now, in order to not do "=== X.class || === Y.class", I have to use an additional reflection operation, isAssignableFrom. Unfortunately, since my code must be emulated by GWT, which does not support isAssignableFrom(), I need another way, probably one that does not use reflection, so my code is compatible with GWT.

Can I design my inventory system differently in a cleaner, non-reflective way? Is this a bad use of reflection that could cause problems later or is the use of reflection not something I should worry about until there are performance/memory issues?

Edit

I should clarify my goals and my design. After further thought, I am not concerned about memory usage or performance. I would like to know if my design is the cleanest and most elegant, or if there is a better design I can use that fit these requirements:

  • Count types of inventory items. I don't need specific parameters so I just need the type. The only other way I could think of was to use
  • Count group of types of inventory item
  • Use a specific instance of these inventory items. Let's say we have 3 routes in inventory. That means I can instantiate and use PlaceRouteCommand 3 times. Each object will have a specific argument of which route, which path, etc.

Put simply, I needed to keep track of the count of each class in a polymorphic collection so I can benefit from polymorphism and distinguish between types within the collection. For this reason, I can't simply use an enum of inventory types (not to mention this violates OCP) because these types need to be grouped together.

For example, in my Inventory class, I have another method that only takes in an InfrastructureItem which is a type of inventory item. Every InventoryItem is either an InfrastructureItem or a Powerup. These are both abstract classes.

21
  • 1
    I always thought an inventory item was a thing - a noun. Not a verb or action. When I see "command" I think "verb" not "noun." Can you clarify what you mean by command here? Commented Jun 27 at 18:17
  • 2
    Your example is a little confusing here. I'm not sure exactly what you mean to show with that. Is that a proposed solution? Can you show an example (or more) that help illustrate your current approach and what you think is problematic?
    – JimmyJames
    Commented Jun 27 at 21:59
  • 1
    @JimmyJames the example is the current implementation without the new feature.
    – Basilevs
    Commented Jun 27 at 22:40
  • 1
    @DocBrown I was going to ask the same thing, but I think it's checking whether the class is-a PowerUp. I'm not sure why the class needs to be passed in, though. It seems like countItem() could do that, assuming this is a member method.
    – JimmyJames
    Commented Jun 28 at 15:42
  • 1
    Seeing the answers here so far, I am pretty sure your solution with reflection is the most simple and clean one among all in this Q&A thread and you should only worry about performance when it becomes a real issue. Not reallly worth to post this as an answer, I think.
    – Doc Brown
    Commented Jul 1 at 6:32

7 Answers 7

2

Another way you can go about this if you, (per DocBrown) want to avoid reflection and want InventoryItem to be agnostic of Powerup, you can try something like this. NOTE: I don't really like this solution.

abstract class Powerup implements InventoryItem {
    private static final Set<Powerup> POWERUPS = 
        Collections.newSetFromMap(new WeakHashMap<Powerup, Boolean>());

    public static final boolean isPowerup(InventoryItem item) {
        return POWERUPS.contains(item);
    }

    Powerup() {
        POWERUPS.add(this);
    }
}

If the compiler hasn't changed too much, I think this is allowed. I kind of hate it, though. Aside from the silliness of storing references just to identify them, I think this approach is a little dangerous in general. The issue is that a reference to this is being exposed outside of the constructor before the object is fully constructed. In this context I think (think) it's probably OK but even then, it's a bit of a slippery slope and might result in some wacky issues in broader usage.

6
  • Wow-wow, use of WeakReferences in game-dev.
    – Basilevs
    Commented Jun 28 at 21:50
  • Both of your answers duplicate mine :)
    – Basilevs
    Commented Jun 28 at 21:52
  • @Basilevs "use of WeakReferences in game-dev" I almost posted an example with a regular set, but I realized that would create a potential memory leak. This approach is problematic enough without that.
    – JimmyJames
    Commented Jul 1 at 16:32
  • 1
    @Basilevs "Both of your answers duplicate mine :)" I can see parallels, but I don't think so. My goal was to show how to avoid using any reflection or class metadata. I thought that was what the OP was looking for but now I'm not sure.
    – JimmyJames
    Commented Jul 1 at 16:37
  • This solution won't work because I have multiple players each with their own bag of inventory items. A static reference can't distinguish between players.
    – Marvin
    Commented Jul 6 at 14:32
1

I think you could flesh out your question a bit around some of the interfaces and classes you mention, but I think have enough of a picture to suggest another option which would fit into your current design with minimal changes. I'm only showing the definitions that relate to the question at hand since you haven't provided details on InventoryItem and Powerup.

interface InventoryItem {
    // ... yada yada

    default boolean isPowerup() {
        return false;
    }

    default PowerUp asPowerup() {
        // TODO: refine as needed
        throw new IllegalStateException("not a power up!");
    }
}


abstract class Powerup implements InventoryItem {
    // ... blah blah blah

    public final boolean isPowerup() {
        return true;
    }

    public final Powerup asPowerup() {
        return this;
    }
}

Then in your code, you can call the isPowerup() method instead of checking whether the object is an instance of Powerup, and if that returns true, call the asPowerup() method to get access to the Powerup without an explicit downcast.

13
  • 2
    This, in principle, is the same suggestion as the one by Basilevs and it has IMHO the same issue: the InventoryFramework will logically depend on the knowledge there exist a specific subclass.
    – Doc Brown
    Commented Jun 28 at 18:26
  • 2
    @DocBrown "the InventoryFramework will logically depend on the knowledge there exist a specific subclass." For sure, given that the InventoryItem interface will explicitly depend on the Powerup class. But the way I see it, if you are essentially checking every InventoryItem's type to see if it is a Powerup, you might as well be honest with yourself and make it explicit. I don't think this is really optimal, but it retains the current design without reflection or casting. A better solution might be pattern matching which isn't typical in Java, to my knowledge (I'm out of the game.)
    – JimmyJames
    Commented Jun 28 at 18:46
  • I knew someone would downvote this. Please tell me what's the issue with this answer, which, as far as I can tell is completely "fulfilling all of these requirements and avoid reflection"?
    – JimmyJames
    Commented Jun 28 at 20:23
  • 1
    Yep, valid reasoning - if Inventory has the responsiblity to count powerups, then this approach is valid. There is a way to avoid both the downcast and most of reflection via Facet, but that's a cringy overkill.
    – Basilevs
    Commented Jun 28 at 21:55
  • 1
    @Marvin And why are you using the class as the key? Is there some requirement in GWT that necessitates that?
    – JimmyJames
    Commented Jul 1 at 16:40
1

Ok, here is a solution that is cringe as heck, is performant and is nauseously flexible. It is taken from C++ locale facet framework. It basically introduces its own enum-like type metadata table.


public final class Facet<T> {
  private static final AtomicInteger count = new AtomicInteger(0);
  final int index = count.getAndIncrement();
  private final Clazz<? extends T> clazz;
  public Facet(Class<? extends T> clazz) {
    this.clazz = Objects.requireNonNull(clazz);
  }
  T downcast(Object object) {
     // Note, this might be counted as reflection
     // Replace with direct unchecked cast for performance
     // Clazz is also unnecessary then
     return this.clazz.cast(object);
  }
}

public class Inventory {
  // Could be a map, but for a fixed type space list works best
  private final List<List<Object>> slots = new ArrayList<>();
  public void add(Object stack, Facet<?> ... facets) {
   for (Facet<?> facet: facets) {
      assert facet.downcast(stack) != null; // replace with true precondition check if desired
      if (slots.size() <= facet.index) {
       slots.addAll(Stream.generate(ArrayList::new).limit(facet.index - slots.size() + 1).toList());
      }
      slots.get(facet.index).append(stack);
   }
  }

  public Collection<T> get(Facet<T> facet) {
    // This may look like unnecessary slowdown, but the copy avoids leaking internal references
    return slots.get(facet.index).stream().map(facet::downcast).toList();
  } 

}


interface InventoryStack {
   void use();
   int count();
)

class PowerUp implements InventoryStack {
 ... 
}

public static final Facet<PowerUp> POWERUP_FACET = new Facet<>(PowerUp.class);
public static final Facet<InventoryStack> STACK_FACET = new Facet<>(InventoryStack.class);

Inventory inventory = ...;

inventory.add(new PowerUp(), POWERUP_FACET, STACK_FACET);

System.out.println("Powerups: " inventory.get(POWERUP_FACET).stream().mapToInt(InventoryStack::count).sum());

System.out.println("Inventory size: " inventory.get(STACK_FACET).stream().mapToInt(InventoryStack::count).sum());

Note, how any inventory object can implement arbitrary number of facets and does not need a special interface for each. This can be used for further categorization, unrelated to type system. For example, rockets, bullets and lasers can implement the same interface, but have distinct facets.

I do not recommend to use this or similar solutions, as for any game of a small to medium size, simpler, less flexible solutions are enough and are much more readable. Such flexibility is only needed in large projects and libraries.

This is just a demonstration, how a type-safe, performant, flexible solution without reflection could be implemented.

2
  • @DocBrown nit this
    – Basilevs
    Commented Jun 28 at 22:48
  • That looks overengineered, and i dont't feel motivated to twist my brain so much until I understand this code.
    – Doc Brown
    Commented Jun 29 at 5:18
0

Avoid reflection altogether, use objects instead of relational model.

class Type ....
interface InventoryStack {
   Type type();
   void use();
   int count();
   bool isPowerUp();
)

class Inventory {
Map <Type, InventoryStack> = ...
}

Method isPowerUp could be moved to Type, but it does not really matter, so I biased for speed against normalization (which is for RDBMS, not OOP).

2
  • 2
    Technically, this will work, but I think this suggestion breaks the idea of making the Inventory infrastructure really independent from the specific subclasses of InventoryItem (at least, logically).
    – Doc Brown
    Commented Jun 28 at 18:09
  • @DocBrown yes, this does cut the corner, but the really flexible approaches are not suited for gamedev, so I settled on this.
    – Basilevs
    Commented Jun 28 at 21:48
0

If you basically need a marker interface, but you cannot use it due to limitations in your chosen framework, just step back a bit and rethink.

As far as I understand your setup, you have some kind of item categories, like powerups, and whatever else you need in your system.

As an item can belong to more than one category, you could simply ask the item for the categories it belongs to.

Thus, a design that comes to mind is:

enum ItemCategory {
    WEAPON,
    ARMOR,
    POWERUP,
    CATALYST,
    CONTAINER,
    FLYING_OBJECT
}

Then, extend the abstract InventoryItem with:

abstract EnumSet<ItemCategory> getCategories(); // or List, if GWT cannot handle EnumSets

... and for each Item implement it:

public class Airplane extends InventoryItem {
    public EnumSet<ItemCategory> getCategories() {
        return EnumSet.of(ItemCategory.CONTAINER, ItemCategory.FLYING_OBJECT);
    }
}
3
  • It would be great to do it this way, but I can't do it for the same reason as a previous answer: I can't call instance or static methods with a class object.
    – Marvin
    Commented Jul 1 at 14:07
  • @Marvin but there are no class objects here. Either add an instance method to enum, or directly compare enums in filtering code.
    – Basilevs
    Commented Jul 1 at 17:55
  • @Marvin I also cannot see any static call or any class-level call here. It's been quite a while since I last worked with GWT (back then, when it wasn't a dead library), but I seem to reccall that simple object-oriented subclassing is not a problem. And that's all we have here: abstract method in the superclass, implementation in every subclass.
    – mtj
    Commented Jul 2 at 5:02
0

Third time is the charm ...

There are possibly other design changes to consider here but I can only make some guesses about that. I'm just going to focus on what you've shown and assume the approach you are taking as a given. I've modified your Inventory class slightly:

class Inventory {
    private final Map<Item, Integer> itemBag;
    // ...
    
    public boolean useItem(Item item) {
        int count = itemCount(item);

        if (count > && item.use(player, gameMap)) {
            itemBag.put(item, count - 1);
            return true;
        } else {
            return false;
        }
    }

    public void addItem(Item item) {
        itemBag.put(item, itemCount(item) + 1);
    }

    public int itemCount(Item item) {
        return itemBag.getOrDefault(item, 0);
    }

    // ...
}

In theory, that could work as is depending on how you are creating the Item instances i.e., you can guarantee only one instance of each Item class is ever created. The issue being that if you create two or more instances of an Item, the inventory count and use function will not work reliably. This is perhaps how you ended up with the design in your question.

There are a few ways to address that. Probably the most idiomatic approach in Java would be to use a 'typesafe enumeration'. In Java, enums are more than just fancy integers or a list of values. They can have methods and overrides much like a normal class. The big upside here is that each enum value has a singular identity which solves the issue above. The downside is that (in my experience) they don't scale well. So, if you to a lot of different item types with a lot of specialized behaviors, you are going to end up with a gigantic enum declaration that (AFAIK) you can't break up into smaller source files. If the number of items is reasonably small, it might be OK.

Another way to deal with this would be to make every Item class a singleton. Everyone hates singletons but I think this might be a valid use case for them to create a sort of enum-esque set of classes. The big problem here is that it requires all the Item types to be written in this way and might require tedious coding. Even if there's some annotation for this that I'm unaware of, I still don't feel great about situations like that where you could forget in one class, and nothing will fail but you get unstable behavior. You could implement a sort of factory which creates all the instances for you but that gets a little hairy with the same kind of reflection and meta-programming I'm suggesting you avoid. It might solve your problem if that is encapsulated and doesn't conflict with GWT requirements, so I won't completely discount it.

Based on what you've explained, I think the easiest answer is to simply to override equals in the InventoryItem class:

public abstract class InventoryItem {
    abstract boolean use(Player player, GameMap game);
    
    @Override
    public final boolean equals(Object other) {
        return this.getClass() == other.getClass();
    }
    
    @Override
    public final int hashCode() {
        return this.getClass().hashCode();
    }
}

And this makes it so that any Item will be considered equivalent to any of Item of the same concrete class. You can combine this with other approaches to avoid unnecessary InventoryItem instantiations as well and this would help protect you from issues with e.g., unmarshalling creating new instances.

Now, you might say: "wait a minute, you are using the class object of the type, what gives?" and you would have a point. I would respond that the difference is that you are doing this once, in a clean, encapsulated place which frees you from worrying about Class objects in your other code. Referencing getClass() in equals() is a standard practice in Java and often recommended. IMO, the biggest issue with your Inventory class as you have it, is that you reference the Class class in your public interface. You could maybe just change the class to this and get a similar result:

class Inventory {
    private final Map<Class<? extends InventoryItem>, Integer> itemBag;
    // ...
    
    public boolean useItem(InventoryItem item) {
        int itemCount = countItem(item);
        if (itemCount < 1)
            return false;

        boolean success = item.use(player, gameMap);
        if (success)
            itemBag.put(item.getClass(), itemCount - 1);
        return success;
    }

    public void addItem(InventoryItem ability) {
        itemBag.put(ability,countItem(ability) + 1);
    }

    public int countItem(InventoryItem item) {
        return itemBag.getOrDefault(item.getClass(), 0);
    }
    // ...
}

But I'm not sure if that addresses your issues with GWT or not.

There are a few other things you might want to consider but I can't give detailed advice on without a larger view of the design. Mainly, I question whether it's necessary or useful to avoid instantiating an InventoryItem for each logical item in the game. I think sometimes people don't realize that a class definition exists in memory exactly onceᵜ no matter how many instances you create. The memory footprint of an object is roughly its what's required to manage its identity and the memory for its variables. It's hard to imagine that you would have so many InventoryItem objects that this would be an issue and it's a much more straightforward way to approach OO. It would also potentially increase the flexibility of the design, e.g., allowing for modifiers on individual items.

ᵜ assuming a single class loader

2
  • It seems you have missed this similar answer softwareengineering.stackexchange.com/a/453922, where the OP deleted it after I told them I don't see any indication in the question for items beeing singletons. Quite the opposite, the OPs intentional use of getClass in the question tells me we should expect more than object of each subclass.
    – Doc Brown
    Commented Jul 4 at 14:32
  • @DocBrown I think maybe you aren't understanding my point around that. I agree there are probably multiple instances but the OP clearly wants to treat them as if they are all the same. They are add to the inventory using the class but use the item by calling with an object. An object that was never added to the inventory. I don't recommend singleton here, as explained in the answer.
    – JimmyJames
    Commented Jul 4 at 18:00
-2

I'm very confused by use of getClass() for stuff that is usually done via Visitor while at the same time worrying about other uses of reflection. But if the design is set in stone, and the only problem is performance, just cache the slow computation.

Map<Class, Boolean> isPowerUp = ...
return isPowerUp.computeIfAbsent(item.getClass(), k -> Powerup.class.isAssignableFrom(k));
2
  • 1
    I dont see how this idea will avoid to call getClass for each call to useItem, which is the part the OP wants to avoid.
    – Doc Brown
    Commented Jun 28 at 18:13
  • @DocBrown The first sentence addresses this. Also, OP stated that they are happy with original design which uses getClass().
    – Basilevs
    Commented Jun 28 at 21:47

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