Collections management with many derived elements

Question

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

---

Testable code

The code below can be copied/pasted in VS as is, to be used for testing purposes

using System;
using System.Collections.Generic;
using System.Text.RegularExpressions;

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

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

    public class Character
    {
        public List<Item> Inventory { get; set; }

        public Character()
        {
            Inventory = new List<Item>
            {
                new Potion
                {
                    Id = 1,
                    Name = "Lesser HP potion",
                    Description = "Heals 42 HP",
                    HPRestored = 42,
                    MaxQuantity = 20,
                    Quantity = 10
                },
                new Potion
                {
                    Id = 2,
                    Name = "Greater HP potion",
                    Description = "Heals 100 HP",
                    HPRestored = 100,
                    MaxQuantity = 20,
                    Quantity = 10
                },
                new Armor
                {
                    Id = 3,
                    Name = "Lesser Armor",
                    Description = "A basic armor",
                    Defense = 10
                },
                new Armor
                {
                    Id = 4,
                    Name = "Regular Armor",
                    Description = "An armor",
                    Defense = 25
                },
                new Armor
                {
                    Id = 5,
                    Name = "Superbe Armor",
                    Description = "A nice shiny armor",
                    Defense = 50
                },
                new Weapon
                {
                    Id = 42,
                    Name = "Lesser Sword",
                    Description = "A basic sword",
                    Attack = 50
                },
                new Weapon
                {
                    Id = 43,
                    Name = "Regular Sword",
                    Description = "A sword",
                    Attack = 75
                },
                new Weapon
                {
                    Id = 44,
                    Name = "Superbe Sword",
                    Description = "A nice shiny sword",
                    Attack = 100
                }
            };
        }
    }

    public class Potion : Item, IUsable, IStackable
    {
        public int HPRestored { get; set; }
        public int Quantity { get; set; }
        public int MaxQuantity { get; set; }

        //This is not exactly how I implemented it, but it's sufficient for testing purpose
        public void Use(Character target)
        {
            --Quantity;
            Console.WriteLine("Used " + Name);
        }
    }

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

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

    public class Armor : Item, IEquipable
    {
        public int Defense { get; set; }
        //This is not how I implemented it, but it's sufficient for testing purpose
        public void TakeOn(Character target)
        {
            Console.WriteLine("Wearing armor : " + Name);
        }
        public void TakeOff(Character target) { }
    }

    public class Weapon : Item, IEquipable
    {
        public int Attack { get; set; }
        //This is not how I implemented it, but it's sufficient for testing purpose
        public void TakeOn(Character target)
        {
            Console.WriteLine("Wearing weapon : " + Name);
        }
        public void TakeOff(Character target) { }
    }

    public class AbsentFromInventory : Item
    {

    }

    class Program
    {
        static Character player = new Character();

        static string ArmorDetail(Armor armor)
        {
            return (armor.Name + " : " + armor.Description + " -> Defense : " + armor.Defense);
        }

        static string WeaponDetail(Weapon weapon)
        {
            return (weapon.Name + " : " + weapon.Description + " -> Attack : " + weapon.Attack);
        }

        /// <summary>
        /// Displays some elements of the inventory depending of its type
        /// </summary>
        /// <typeparam name="T"></typeparam>
        /// <param name="type">This is only for friendly display purpose</param>
        static void SpecificInventoryItems<T>(string type) where T : class
        {
            string error = "";
            string input;
            List<T> items;
            bool back = false;

            do
            {
                do
                {
                    items = null;
                    items = new List<T>();
                    string options = type + " :\n";
                    int i = 1;
                    if (player.Inventory.Count > 0)
                    {
                        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.MaxQuantity + ") ";
                                }
                                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);
                            }
                        }

                        if (i > 1)
                        {
                            Console.WriteLine(options);
                            if (error != "")
                                Console.WriteLine(error);
                            Console.WriteLine("Please select an item or go [[B]ack]");
                        }
                        else
                            Console.WriteLine("No " + type.ToLower() + " found. Please go [[B]ack]");
                    }
                    else
                        Console.WriteLine("No item in inventory. Please go [[B]ack]");
                    input = Console.ReadLine();
                    error = "";

                } while (!Regex.IsMatch(input, "^(([0-9]+)|(b(ack)?))$", RegexOptions.IgnoreCase));

                if (int.TryParse(input, out int result))
                {
                    if (result > 0 && result <= items.Count)
                    {
                        var selected = items[--result];
                        switch (selected)
                        {
                            case IEquipable eq:
                                eq.TakeOn(player);
                                back = true;
                                break;

                            case IUsable us:
                                us.Use(player);
                                if (us is IStackable)
                                {
                                    var stackable = us as IStackable;
                                    if (stackable.Quantity <= 0)
                                        player.Inventory.Remove(stackable as Item);
                                }
                                else
                                    player.Inventory.Remove(us as Item);
                                back = true;
                                break;

                            default:
                                break;
                        }
                    }
                    else
                        error = "Invalid number.";
                }
                else
                    back = true;
            } while (!back);
        }

        static void Main(string[] args)
        {
            do
            {
                SpecificInventoryItems<Potion>("Potion");
                SpecificInventoryItems<Weapon>("Weapon");
                SpecificInventoryItems<Armor>("Armor");
                SpecificInventoryItems<AbsentFromInventory>("Super item");
                Console.WriteLine("Again? Y/N");
            } while (Console.ReadKey().Key != ConsoleKey.N);
        }
    }
}

Answer 1

About 'is'

I see four different uses of is in your code:

1. item is T, for filtering on item type, such as weapons-only or potions-only.
2. item is Armor and item is Weapon, for creating type-specific effect descriptions.
3. item is IStackable, for descriptions and special inventory handling of stackable items.
4. case IEquipable eq and case IUsable us, for handling of equipment and consumption logic that applies to certain item 'categories' (both weapons and armors are equippable, only potions are consumable).

---

Number 1, selecting items of a specific type from a collection of different types (which is a very natural way to represent a player's inventory) seems fine. Note that this can be simplified by using Linq's OfType<T> extension method. An alternative such as using separate lists will likely introduce more problems than that they solve.

---

Number 2 is problematic. You can easily end up with several checks like these in multiple places, which makes updating description or other item-specific logic more difficult and error-prone than it needs to be. A better solution is to add a virtual EffectsDescription property to Item, so each item type can generate its own effects description, using actual effect values. Adding a new item type no longer requires changes in various other parts of the code, because type-specific description logic is kept in a single place, the item class itself.

---

Number 3, stackable item handling, looks ok at first sight, but it's not very flexible. A class either implements an interface or it does not, so each item type is either stackable or it's not. A two-handed broadsword isn't stackable, but what about a set of throwing knives? The current design does not allow for special cases like that. I would use a data-driven approach here instead of encoding this in C#'s type system. The easiest solution would be to make all items stackable, with a max stack size of 1 for unstackable items.

---

Number 4, equippable and consumable item handling, can sometimes be useful, but in this case I think it indicates a suboptimal design choice. A call like SpecificInventoryItems<Weapon> is intended to let the player select a weapon, but because SpecificInventoryItems is a generic method it has to resort to type-checks to determine how to use the selected item - something that could easily (and unambiguously) have been decided by the caller. I would split this up into a generic SelectItem<T> method and several type-specific methods such as EquipWeapon.

---

Other notes

• Pattern matching isn't limited to switch statements: if (item is Armor) ...item as Armor can be rewritten as if (item is Armor armor) ...armor. That also reduces the number of type checks to one.
• SpecificInventoryItems is doing too many things. Splitting it up into smaller functions that each perform a single task not only makes the code easier to understand, it also enables you to reuse functionality in different contexts.
• Item.Description is used inconsistently: potions use it to describe their effects, but weapons and armor do not. Note how potion descriptions can easily become incorrect if you change the value of HPRestored but forget to update Description accordingly.
• The T : class constraint does not prevent unrelated interfaces and types. A top-level IItem interface that is implemented by both Item and the various equippable/usable interfaces allows for a more specific constraint, which makes the intent of the method more clear. Renaming T to TItem would also help.
• If you're using options the way you do to reduce the number of Console.WriteLine calls, and if that's because of a concern for performance, then you should know that using a StringBuilder is more efficient than concatenating lots of strings. Not that I'm expecting performance problems here, but just in case you didn't know.
• All Item properties have public setters. This allows any code to modify them, with the risk of putting an item in an invalid state, with the result that a modification in one part of the program can cause another unrelated part to break.

---

Alternative implementation

Here's what the code could look like after applying some of the above suggestions:

// Top-level item-specific methods:
static void EquipWeapon(Character player)
{
    var selectedWeapon = AskUserToSelectItem<Weapon>(player, "weapon");
    if (selectedWeapon == null)
        return;

    // NOTE: Unequipping current weapon left out.
    selectedWeapon.TakeOn(player);
    Console.WriteLine($"{selectedWeapon.Name} has been equipped.");
}

static void EquipArmor(Character player)
{
    var selectedArmor = AskUserToSelectItem<Weapon>(player, "armor");
    if (selectedArmor == null)
        return;

    // NOTE: Unequipping current armor left out.
    selectedArmor.TakeOn(player);
    Console.WriteLine($"{selectedArmor.Name} has been equipped.");
}

static void ConsumeUsableItem(Character player)
{
    var selectedUsableItem = AskUserToSelectItem<IUsable>(player, "consumable");
    if (selectedUsableItem == null)
        return;

    RemoveFromInventory(player, selectedUsableItem, 1);
    selectedUsableItem.Use(player);
    Console.WriteLine($"{selectedUsableItem.Name} has been consumed.");
}

// Displays all items of the specified type.
// Returns the selected item, or null if no item was selected.
static TItem AskUserToSelectItem<TItem>(Character player, string itemDisplayName) where TItem : IItem
{
    var items = player.Inventory.OfType<TItem>().ToArray();
    if (items.Length == 0)
    {
        Console.WriteLine($"Your inventory contains no {itemDisplayName}.");
        return default;
    }

    Console.WriteLine($"{itemDisplayName}:");
    for (int i = 0; i < items.Length; i++)
    {
        Console.WriteLine($"[{i}] - {FullItemDescription(items[i])}");
    }
    var selectedIndex = AskUserForIntegerInRange($"Please select an item or go [[B]ack]:", 0, items.Length - 1);
    return selectedIndex.HasValue ? items[selectedIndex.Value] : default;
}

static void RemoveFromInventory(Character player, IItem item, int quantity)
{
    // NOTE: Ownership and quantity sanity check left out for brevity's sake:
    if (item.Quantity > quantity)
        item.Quantity -= quantity;
    else
        player.Inventory.Remove(item);
}

static string FullItemDescription(IItem item)
{
    var description = new StringBuilder();
    description.Append(item.Name);

    if (item.StackSize > 1)
        description.Append($" ({item.Quantity}/{item.StackSize})");

    description.Append($": {item.Description} -> {item.EffectsDescription}");
    return description.ToString();
}

// Asks the user to enter a number within the given range.
// Returns null if the user entered 'b' or 'back'.
static int? AskUserForIntegerInRange(string prompt, int minValue, int maxValue)
{
    while (true)
    {
        Console.WriteLine(prompt);
        var input = Console.ReadLine().ToLower();
        if (input == "b" || input == "back")
        {
            return null;
        }
        else if (int.TryParse(input, out var index))
        {
            if (index >= minValue && index <= maxValue)
                return index;

            Console.WriteLine($"Please enter a number between {minValue} and {maxValue}.");
        }
        else
        {
            Console.WriteLine("Please enter a valid number, or 'b' or 'back'.");
        }
    }
}

With the following changes to the Item class hierarchy (not including everything, but the rest should be obvious):

public interface IItem
{
    int Id { get; }
    string Name { get; }
    string Description { get; }
    string EffectsDescription { get; }
    int Quantity { get; set; }
    int StackSize { get; }
}

public abstract class Item : IItem
{
    ...
    public virtual string EffectsDescription { get; }
}

public interface IEquippable : IItem { ... }

public class Weapon : Item, IEquippable
{
    ...
    public override string EffectsDescription => $"Attack: {Attack}";
}

Perhaps not all of these changes will make sense for what you have in mind with your game, but I hope you'll find some of this useful.