3

To start off, this is more of a best-practice question than anything.

Introduction to the Environment

I have myself an abstract class. Let's call this class Item. I also have another abstract class, we'll call this one Container. The Container class inherits the Item class.

Here's my dilemma: I'm trying to right-properly design the OOP portions of it, and I'm having a bit of an issue due to my desire to abstract away as much as possible, while still creating maintainable and understandable code.

Let's start with some code:

public abstract class Item
{
    private Guid _Guid;
    private float _Weight;

    public Guid Guid { get { return _Guid; } set { _Guid = value; } }
    public float Weight { get { return _Weight; } set { _Weight = value; } }

    public virtual ItemType ItemType { get { return ItemType.None; } }
}

public abstract class Container : Item
{
    private float _MaxWeight;
    private List<Item> _Items;

    public List<Item> Items { get { return _Items; } }
    public float MaxWeight { get { return _MaxWeight; } set { _MaxWeight = value; } }
    public abstract ContainerType ContainerType { get; }

    public sealed override ItemType ItemType { get { return ItemType.Container; } }
}

So, with this code the intention is for a Container to be able to be treated as an Item, with a few exceptions. (The container will have additional properties, etc.)

The issue, is that not all Item objects can be added to a Container. In fact, only Container objects that have a smaller Container.MaxWeight or non-Container Item objects can be added.

As of now, this restriction cannot be placed. My initial thought was to create an ItemCollection inside the Container class, that would essentially hold all of the Item objects within it. This would allow me to implement my own ItemCollection.Add method which would perform this filter.

The issue comes into play when I introduce my third class. Let's call it a Backpack. This inherits Container and looks as follows:

public class Backpack : Container
{
    public sealed override ContainerType ContainerType { get { return ContainerType.Backpack; } }
}

Mind you - the Backpack (as we are calling it) will only be able to hold Item objects, or Container objects with a smaller Container.MaxWeight in them.

This also means that I may yet have other classes (let's use, for example, Pouch) that will also inherit Container, but which cannot contain other Container objects.

Lastly, there may also be additional classes that cannot hold Item objects, but only hold Container objects. (See, for example, Inventory.)

/// <summary>
/// Represents a collection of <see cref="Container"/> objects owned by an <see cref="Actor"/>.
/// </summary>
public class Inventory : Container
{
    public sealed override ContainerType ContainerType { get { return ContainerType.Inventory; } }
}

Question Statements

Is this the best way to do it? Should I implement the ItemCollection as I intended? What about Container objects that can only hold other Container objects?

Should I use the ContainerType.Inventory to indicate that this object can only hold other Inventory objects? Should I use the ContainerType.MaxWeight value on it instead? (Any objects that can only hold Container objects won't really have a MaxWeight. The MaxWeight an Actor can hold is determined by their fitness.)

Should I make an additional class that inherit Container that can only hold other Container objects, then inherit Inventory from this? Likewise, should I create another class inheriting from Container that allows or disallows the addition of Item objects only?

Additional Notes/Jargon

The idea is to force the programmers (mostly me) to properly handle each Container and Item as they should be. The issue lies in the fact that there are different types of Container objects. Also, do note that this is not all of the code involved, but the additional code I excluded is irrelevant to this issue. Also note, the two additional classes I mentioned (Inventory and Backpack) are completely concrete. I will be working directly with those. I also want to have common infrastructure between them, so that any Item that is a Container can be worked in a general manner. (Rather than casting to the appropriate concrete class.)

  • 1
    ericlippert.com/2015/04/27/wizards-and-warriors-part-one (and the followup articles) may be of interest. They deal with the limitations of using the type system to represent arbitrary rules. – Ben Aaronson Jun 26 '15 at 18:43
  • @BenAaronson I didn't think about representing this information as disparate systems like that, I think that may be the ticket here. – Der Kommissar Jun 26 '15 at 19:03
  • 1
    Honestly I've never really worked on a system like that, so I don't know whether it's appropriate to your situation. But it's at least worth keeping the idea in the back of your mind so if you start to feel like you're bending over backwards to coerce the type system to represent your rules, you'll be aware of it – Ben Aaronson Jun 26 '15 at 19:09
  • @BenAaronson That's almost what it's been feeling like - I think that the approach Eric outlines there, combined with the approach Alexus outlined in his answer might be my best bet. Treating this as a rules-commands-effects situation, rather than an object-action situation might do me some good. – Der Kommissar Jun 26 '15 at 19:11
2

The direction where you are going is great. I would make a few tweaks to this design like so:

public abstract class Container : Item
{
    private float _MaxWeight;
    private List<Item> _Items;

    public List<Item> Items { get { return _Items; } }
    public float MaxWeight { get { return _MaxWeight; } set { _MaxWeight = value; } }
    public abstract ContainerType ContainerType { get; }

    public IEnumerable<IDepositFilter> Filters { get; set; } // Probably a factory can stuff filters as required per each type.

    public void DepositItem(Item item) {
        foreach(var filter in Filters) {
            if(!filter.IsAllowed(item)) {
                throw new ItemNotAllowedException();
            }
        }

        _Items.Add(item);
    }
}

public interface IDepositFilter
{
    bool IsAllowed(Item item);
}

public class DefaultContainerFilter : IDepositFilter {
    // probably do nothing here
}

public class OversizeContainerFilter : IDepositFilter
{
    // Check if the size of the new deposit is larger than leftover space in this container
}

public class ItemTypeFilter : IDepositFilter
{
    // check what is being deposited and allow/disallow accordingly.
}

Notice, I introduced a filter collection that should be used when you are adding something to your container. You can add as many filters as you want to your container of a specific type. Each filter does one and only one check.

I also remove the ItemType as it seems redundant - you can use classes themselves to determine the type.

Reuse the filters on anything you want :)

Hope this helps.

  • 1
    I suspect you may bump up against problems here. Like if you get passed an Item, if it's a Container you need to check its max weight. That would mean the filter would need to try casting every item to container. This gets potentially messy and unweildy if the class hierarchy grows. – Ben Aaronson Jun 26 '15 at 18:46
  • 1
    @EBrown I thought you also had a rule that a container can't be stored in a container with a lower MaxWeight than its own, even if the actual Weight is sufficiently smaller. – Ben Aaronson Jun 26 '15 at 18:49
  • 1
    @EBrown But container weight is not a container MaxWeight, max weight can grow and empty huge box will fit in your little back pack, until you stuff the box to the max... so logically you need to comapare ParentContainer.Weight >= DepositedContainer.MaxWeight, – Alexus Jun 26 '15 at 18:54
  • 1
    @Ebrown, Right, but what Ben is saying is that in order to check what's the max weight of the item that is being added is, you need to cast the Item into a specific type of Container to gain access to MaxWeight variable. Your deposited container might not weight much at the time of addition, but if you add something to your child container to max out its weight AND it's max weight is more than your parent container weight, you got yourself a logic issue. – Alexus Jun 26 '15 at 19:16
  • 1
    Last thing, you might as well have a composite filter that stacks a bunch of filters inside and handles the logic to determine if the item is a container inside it and apply sub-filters to the item respectively. So you can create as many levels of filters as you want with this approach. – Alexus Jun 26 '15 at 19:23
2

Unfortunately, exposing public List Items {get;} breaks the whole encapsulation and protection mechanisms. The using code can just do container.Items.Add and put whatever they want. (which is a problem in the other answer as well).

More and more I start to prefer composition instead of inheritance, and when some form of inheritance is needed (to avoid duplicated code), a decorator pattern may come in place.

And for the sake of the argument (I understand that the code sample is not complete), I do not see a reason of using an abstract class to start with. Interface may be more appropriate, if we want just to describe what properties an item can have.

Although the above is the way to go, if there is a "real" need to use inheritance, here is an example code how I would approach this:

public abstract class Item
{
   public string SomeData{get;set;)
}

public abstract class Container : Item
{
    private readonly List<Item> _items;
    public IEnumerable<Item> Items { get { return _items;}}
    public Container()
    {
       _items = new List<Item>();
    }

    public void AddItem(Item item)
    {
       if (IsValidItem(item))
       {
           _items.Add(item);
       }
    }

    protected virtual bool IsValidItem(Item item)
    {
       ///some validation logic, etc
    }
}

With the above very simplistic/schematic design its easy to achieve the questions from the original post. In the other types of containers, it is about overriding the protected IsValid method to do whatever it need to be (incl if it has to call the base method, etc.).

Also, we do not break the encapsulation, so external consumers of the class can not add items w/o validation.

When questions start to come about reseting the container, removing items, etc., probably it's better to make your container class to inherit from Item, and implement ICollection interface, using a backing storage of internal list, and forward (trough some validation) the corresponding calls to it:

public class Container : Item, ICollection<Item>

NOTE: As I said earlier, I will not go with this approach, it is a nightmare to test and extend. And it probably in the real world implementation is going to break one or more of the SOLID principles.

  • This is similar to what I ended up with. (Though, I had Container inherit from Item, IEnumerable, and IList<Item>. I then implemented them all to modify the _Items in that manner. (Instead of exposing the List<Item>, I keep it internal and allow the implementer to treat a Container as as List<Item>.) – Der Kommissar Jun 26 '15 at 21:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.