lundi 10 décembre 2018

Collections management with many derived elements

I've been told few days ago that using is is a code smell and anti-pattern.

I used it in an game to filter some elements of a collection (the inventory of the player).


Classes structure

The base class of an element of the collection is Item :

public abstract class Item
{
    public int Id { get; set; }
    public string Name { get; set; }
    public string Description { get; set; }
}

From that class are derived 3 classes, Potion, Armor and Weapon

A potion is IUsable

public interface IUsable
{
    void Use(Character target);
}

And IStackable

public interface IStackable
{
    int Quantity { get; set; }
    int MaxQuantity { get; set; }
}

The class Potion is, in example, defined that way :

public class Potion : Item, IUsable, IStackable
{
    //Some properties and implementations of interfaces
}

The weapons and armors are IEquipable

public interface IEquipable
{
    void TakeOn(Character target);
    void TakeOff(Character target);
}

Here is a shortened example of Armor

public class Armor : Item, IEquipable
{
    //Some properties and implementations of interfaces
}

And one for Weapon

public class Weapon : Item, IEquipable
{
    //Some properties and implementations of interfaces
}


Collection access

Now, the part that smells, considering what was told to me :

To access some specific items in the collection, I'm using a generic method SpecificInventoryItems<T>(), which contains this part to get the expected T items :

foreach (Item item in player.Inventory)
{
    if (item is T)
    {
        string stack = "";
        if (item is IStackable)
        {
            var stackable = item as IStackable;
            stack = "(" + stackable.Quantity + "/" + stackable.MaxAmount + ") ";
        }
        options += "[" + i++ + "] - " + item.Name + " " + stack + ": " + item.Description;
        if (item is Armor)
            options += " " + ArmorDetail(item as Armor);
        else if (item is Weapon)
            options += " " + WeaponDetail(item as Weapon);
        options += "\n";
        items.Add(item as T); //items is List<T>
    }
}

What's wrong with it? How could (should) I redesign this structure ? I though using separates lists such as List<IUsable> and so on, instead of just a List<Item> Inventory

Another example, after displayed the specific elements of the inventory, the user can select an item, and depending of its type, the item will be either used or taken on :

switch (selected) //selected type is T
{
    case IEquipable eq:
        eq.TakeOn(player);
        break;

    case IUsable us:
        us.Use(player);
        //Manage the deletion of the IUsable if not IStackable, or IStackable and quantity = 0
        break;

    default:
        break;
    }
}

Aucun commentaire:

Enregistrer un commentaire