- Home /
How to Simplify My Drag and Drop Inventory
Hello all,
The inventory system I'm working on has a drag-and-drop functionality, which includes the ability to drag an item onto another, swapping their positions. Unfortunately what I have ended up with is a mess of if statements that I know can be simplified. I've tried a couple of different options including using enums to define states and even writing a state machine. Both of these have failed to simplify the code.
It works fine, but it is screaming to be refactored.
Any ideas?
public void OnEndDrag(PointerEventData eventData)
{
InventoryItem currentInventoryItem = _inventory.FindInventoryItemFromUIElement(this);
if (currentInventoryItem != null)
{
GameObject objectLandedOn = eventData.pointerEnter;
InventoryUISlot uiSlotLandedOn = null;
InventoryItem itemLandedOn = null;
if (objectLandedOn != null)
{
uiSlotLandedOn = objectLandedOn.GetComponent<InventoryUISlot>();
itemLandedOn = _inventory.FindInventoryItemFromUIElement(uiSlotLandedOn);
}
if (uiSlotLandedOn == null)
{
_inventory.DropItem(currentInventoryItem);
}
else if (uiSlotLandedOn != null)
{
bool clearCurrentUIElement = itemLandedOn == null;
if (currentInventoryItem.item.stackSize > 1 && uiSlotLandedOn != InventoryUIManager.activeInventorySlot)
{
if (itemLandedOn != null)
{
itemLandedOn.SetNewUIElement(this, clearCurrentUIElement);
}
currentInventoryItem.SetNewUIElement(uiSlotLandedOn, clearCurrentUIElement);
}
else if (currentInventoryItem.item.stackSize == 1)
{
if (itemLandedOn == null)
{
if (uiSlotLandedOn == InventoryUIManager.activeInventorySlot)
{
_inventory.SetActiveItem(currentInventoryItem, false);
}
else if (currentInventoryItem.uiElement == InventoryUIManager.activeInventorySlot)
{
_inventory.DeleteActiveItem();
}
}
else if (itemLandedOn != null)
{
if (uiSlotLandedOn == InventoryUIManager.activeInventorySlot)
{
_inventory.SetActiveItem(currentInventoryItem, false);
}
else if (currentInventoryItem.uiElement == InventoryUIManager.activeInventorySlot)
{
_inventory.SetActiveItem(itemLandedOn, false);
}
itemLandedOn.SetNewUIElement(this, clearCurrentUIElement);
}
currentInventoryItem.SetNewUIElement(uiSlotLandedOn, clearCurrentUIElement);
}
}
ResetDragObject();
}
Also, to clarify, the "InventoryUIManager.activeInventorySlot" is the UI slot that holds the "active item" or the item the player is holding
Simplifying the code can mean a lot of things but if you want it to be more readable, more debuggable, more understandable, easier to enhance, more modular and dynamic, then break down your too long method(s) to multiple smaller methods (and I mean like max 1-3 lines of code) that have single & simple purpose and functionality. Also name them so that it's clear what they are doing. It may seem at first illogical or hard work to do so, but you can be certain that this kind of methodology will make a lot easier to build, maintain and develop complex systems (like inventory etc..).. way more!
When you have done it and you look at the code that describes the actual inventory logic and it will start to look like "well written prose" (quote from Grady Booch) rather than hard to understand gibberish, you know you're in the right track :) .. give it a try, you won't regret it!
Thanks for the quick reply!
I see how I was a little ambiguous there. I'm thinking there must be a better way to handle what I'm trying to accomplish than a series of if statements, but I'm not really sure how. I could break it down into smaller methods, but that would still mean I have a bunch of if statements, they would just be dispersed throughout several methods.
I'm trying to figure out if there's a different way of approaching this than using so many if statements.
I kind of know what you mean but there is no short answer to this (At least I'm not able to give it). I really suggest to invest time to watch Uncle Bob's presentation about clean code https://www.youtube.com/watch?v=7EmboKQH8lM. Starting from about 60$$anonymous$$. there is example of similar issue that you have and also the solution explained, but I suggest to watch the whole thing :)
Clean code principles has at least for me solved pretty much all of the if - else hell issues (and lot of other things). Even if I sometimes end in a situation where I just write too much logic to one method I soon rationalize it with clean code principles. I not doing it because I'm somehow "clean code purist" but because I see the positive results.
Answer by benjmitch · Nov 21, 2021 at 11:37 PM
I agree with CodesCove that following Uncle Bob's advice is a good place to start. I'm not quite as purist as Bob advises, primarily because I don't do test-driven development, but I learned a lot from his presentation. Linked again here: Uncle Bob
Is there a better way than a "series of if statements"? If blocks aren't bad, but nested if blocks, unless they are entirely symmetric, are error-prone. If there were no nesting, you could recast this whole event handler as a series of independent handlers, each testing for a specific set of conditions surrounding the event, and each terminating execution if it handles the event (i.e. each event is handled by exactly one handler, or none). This pattern is easy to read and debug.
But you have nested conditions! And, code blocks that should run for multiple handlers. These can both be reasonably intuitively handled using child functions within that framework, or you might - if there are relatively few nested cases - choose to unroll (unnest) their conditions, in order to maintain the purity of the 'flat list of handlers' pattern, for the sake of your sanity. Common code blocks can be handled as sub-functions - not perfect, but very clear. There's some flex, here, it depends on the scale of your problem, and how independent the handler conditions actually are.
I've included some example code, below - I probably messed up your logic, but hopefully it's clear. Each event handler is if (conditions) { respond; terminate; }
and each one is mostly independent, though conditions do accumulate as you move down the list - that's not perfect, but it's manageable; you could choose to unroll everything if you prefer to see it all made explicit - in that case, one function per handler, as advised by Bob, probably actually is the way to go. I've added a bit at the bottom to show what a purist version might look like, if performance isn't an issue (and it probably isn't for a UI event handler).
What's changed versus your code? A variety of conditional constructs have all been replaced with a single conditional pattern, so you don't have to figure out the nesting of ifs and elses to understand the code flow, something that can be error-prone (perhaps why you're seeking an alternative solution). You'll notice that there are no else blocks, for example.
Since you mention it, trying to simplify this code flow with enums is, I imagine, going to force you to code those conditions elsewhere, associate enum values with each condition, and then have a switch statement to select a handler. That actually makes things worse - the pattern is the same except that the conditions are now maintained somewhere other than where the handler is. Conditions are often tightly tied to the actions a handler takes - certain resources have to be available, as a key example - so it usually makes perfect sense to have the condition hard-coded at the top of each handler block. Gets more complex if conditions are used in multiple places, but actually that's rare. Ach... YMMV.
public void OnEndDrag_Pragmatic(PointerEventData eventData)
{
InventoryItem currentInventoryItem = _inventory.FindInventoryItemFromUIElement(this);
// handle non-null end-drag inventory item condition
if (currentInventoryItem != null)
{
OnEndDrag_InventoryItemHandler(new ItemData(currentInventoryItem));
ResetDragObject();
}
}
class ItemData // give this a more meaningful name
{
/*
I did this as a class because it makes passing control to sub
functions much cleaner. No need if there'll be no nesting, though,
it only makes your code longer in that case.
*/
public InventoryItem currentInventoryItem;
public GameObject objectLandedOn;
public InventoryUISlot uiSlotLandedOn;
public InventoryItem itemLandedOn;
public ItemData(InventoryItem currentInventoryItem)
{
this.currentInventoryItem = currentInventoryItem;
objectLandedOn = eventData.pointerEnter;
if (objectLandedOn != null)
{
uiSlotLandedOn = objectLandedOn.GetComponent<InventoryUISlot>();
itemLandedOn = _inventory.FindInventoryItemFromUIElement(uiSlotLandedOn);
}
}
}
void OnEndDrag_InventoryItemHandler(ItemData itemData)
{
// handle drop and terminate
if (itemData.uiSlotLandedOn == null)
{
_inventory.DropItem(itemData.currentInventoryItem);
return;
}
// CONDITION MET: itemData.uiSlotLandedOn != null
// handle clear UI element and terminate
bool clearCurrentUIElement = itemData.itemLandedOn == null;
if (itemData.currentInventoryItem.item.stackSize > 1 && itemData.uiSlotLandedOn != InventoryUIManager.activeInventorySlot)
{
if (itemData.itemLandedOn != null)
{
itemData.itemLandedOn.SetNewUIElement(this, clearCurrentUIElement);
}
itemData.currentInventoryItem.SetNewUIElement(itemData.uiSlotLandedOn, clearCurrentUIElement);
return;
}
// handle stack size of not 1 and terminate
if (itemData.currentInventoryItem.item.stackSize != 1)
{
return;
}
// CONDITION MET: itemData.currentInventoryItem.item.stackSize == 1
// handle set active item and terminate
if (itemData.itemLandedOn == null && itemData.uiSlotLandedOn == InventoryUIManager.activeInventorySlot)
{
_inventory.SetActiveItem(itemData.currentInventoryItem, false);
PerformOperationCommonToSeveralHandlers(itemData);
return;
}
// handle delete active item and terminate
if (itemData.itemLandedOn == null && itemData.currentInventoryItem.uiElement == InventoryUIManager.activeInventorySlot)
{
_inventory.DeleteActiveItem();
PerformOperationCommonToSeveralHandlers(itemData);
return;
}
// handle no action and terminate
if (itemData.itemLandedOn == null)
{
PerformOperationCommonToSeveralHandlers(itemData);
return;
}
// and so on...
}
void PerformOperationCommonToSeveralHandlers(ItemData itemData)
{
itemData.currentInventoryItem.SetNewUIElement(itemData.uiSlotLandedOn, clearCurrentUIElement);
}
public void OnEndDrag_Purist(PointerEventData eventData)
{
ItemData itemData = new ItemData(_inventory.FindInventoryItemFromUIElement(this));
if (
Handler_DropItem(itemData)
|| Handler2(itemData)
|| Handler3(itemData)
)
{
ResetDragObject();
}
}
bool Handler_DropItem(ItemData itemData)
{
// condition: require non-null inventory item
if (itemData.currentInventoryItem != null)
return false;
// condition: require null ui slot landed-on
if (itemData.uiSlotLandedOn != null)
return false;
// action and terminate
_inventory.DropItem(itemData.currentInventoryItem);
return true;
}
I really appreciate this answer, man. I actually solved the problem by creating some query functions which lumped a lot of those if statements together. I think my biggest problem was having repeat code depending on whether or not the player was dragging the active item. I was able to simplify that big ol' mess I made into this.
if (CanPerformDragAndDrop())
{
bool swapItems = !_uiSlotLandedOn.isEmpty;
_currentItem.SetNewUIElement(_uiSlotLandedOn);
if (!swapItems)
{
ClearUISlot();
}
else if (swapItems)
{
_itemLandedOn.SetNewUIElement(this);
}
if (DragAndDropInvolvesActiveItems())
{
_inventory.CheckActiveSlotStatusAndUpdateActiveItem();
}
}
Also good - there's always more than one way to solve a problem. You've followed the essential Uncle Bob principle there, of extracting code into well-named functions, which always helps. You can replace 'else if (swapItems)' with 'else', in the code pasted here.
Your answer
Follow this Question
Related Questions
How to split the objects in the inventory 0 Answers
Deleting item from inventory system... 1 Answer
Stack Problem(Please help!) 0 Answers
Inventory system help - getMouseButtomDown problem 2 Answers