How to Simplify My Drag and Drop Inventory

Questions & Answers
1

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();
            }
2

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;
}