Too much reliance on events? Inventory/Shop/Item system


These are the events located on a class called item stack. I tried using interfaces and abstract classes before but it ended up becoming quite messy when a few of the dependents are missing.

My goal with this is to be able to move these stacks around so classes that have a use for them can look for them rather than the other way around. One example would be a class that holds the players stats, when onmodify gets called the stats then get shifted around based on whats removed and replaced.

What I dont like is how the events are completely exposed and I feel that its a bit volatile if I accidentally add an event twice or forget to unsubscribe them. Thankfully, this isn’t a monobehavior so I could just create a new one from scratch if need be.

For items being added/removed/quantity modified, you can just use one delegate and pass an object that outlines the change.

As for OnSendRequest, OnRecieveRequest… why? Why are these delegates? These should just implementations that should be handled internally by any given item container. I’m not sure why this has to be handled by a stack of items itself.

Also you should consider using the event keyword (thus actually making these events, as opposed to just delegates), if these are only ever invoked by their parent type.

Those were misnamed, OnRequestSend makes more sense. The reason why I wanted them to be located in their classes is for convenience. I can do if(OnRequestSend == null) then pass it off as true so it won’t need to rely on any other class to operate.

Still doesn’t really make sense to me for any of these to be on the item stack itself. All it should care about is what item it has, and how many. The item itself should express properties as to it’s maximum stack size, which the stack can help enforce. Otherwise the container should handle the implementation as to whether any more items can be added to it, and informing of any changes to its contents.

What do you use for containers with different logic? Abstract classes or interfaces?

I just have a primary interface that outlines the core implementation of an item container. I believe I’ve shown it before:

public interface IItemContainerContentsData : IReadOnlyList<IReadOnlyItemListing>
{
	IContainerIdentifier ContainerIdentifier { get; }
	
	bool IsHiddenContainer { get; }
	
	event Action<ContainerChangedEvent> OnContainerContentsChanged;
	
	ItemCanAddResult CanAddItem(ItemAddRequest addRequest);
	
	ItemAddResult TryAddItem(ItemAddRequest addRequest);

	ItemRemoveResult TryRemoveItem(IItemRemoveRequest removeRequest);
	
	void ClearContents();

	new ReadOnlyListingEnumerator GetEnumerator();
}

Using an interface namely because I have a wide variety of containers with specific requirements. Though in the end you can still do the same thing with a base class.

Thank you. Do you have the code for one of the Try methods and this event Action OnContainerContentsChanged;
?

I think what I’m trying to do is not having to rely on one specific container for many functions. Let’s say equipment of baseclass itemcontainer, I want it to modify stats of the PlayerCharacterStats class whenever something changes within it without reliance on any UI or other classes. Same thing for all shop classes, there might be more than one shop so how can I ensure that each shop uses PlayerCurrency without actually referencing it?

Basically, I dont want these item classes to know anything besides items, no references to player or anything else other than items, which is why I’m relying on callbacks.

Also, how you you tie buttons and UI into these classes? Are you making a seperate class for each one or is the UI code in the same class?

They’re massive and specific to my project. They probably won’t be of much help. And the most basic implementation is zero restriction of what can go in and out of an inventory. Ergo they have infinite storage. Everything goes in, though you can only take out what exists in the inventory.

All that should happen on a layer above the inventory contents. If you want item containers to do nothing but contain items… then allow them to only do that. Then everything else that cares about specific items happens above that. Or they express more interfaces if they have special properties that other systems may care about.

I will note that the more specific implementations are rather coupled to their usages. The item container for the player’s specific inventory does in fact have a reference to the player (or at least, the player’s storage capacity stat). But because we’re behind an interface, nothing else actually has to care about what it does specifically.

Same as above, having this abstraction means that UI can and should be able to interact with any container contents generically.

An inventory’s contents is just data. The UI takes this data, and presents it to the user. The data has zero involvement in this.


I went with something like this based on your example and something I could understand. It’s not as advanced but it’s quite compact and I’m happy with it for now.

1 Like