7
\$\begingroup\$

After reading a lot about interfaces, which I'm new to, I've tried implementing it in my inventory system for a game I'm working on. I have managed to create something proof of concept which seems to work as intended, but I wanted to check with more experienced C# programmers if this is the correct/best approach?

The player has an inventory, an inventory has several slots, and each slots can have several items. The player can use any of the different items (or, more precisely, item types), but how they are used differs; food items can be eaten, weapon items can be equipped etc.

So I have started with this:

public interface IUser<Item> {
    void Use ( Item usableItem );
}

public class Player : IUser<Item> {
    public float Energy { get; set; }

    public Player () {
        Energy = 0.0f;
    }

    public void Use ( Item usableItem ) {
        usableItem.Use ( this );
    }
}

public interface IUsable<Player> {
    void Use ( Player player );
}

public abstract class Item : IUsable<Player> {
    public string Name { get; set; }

    public Item () {
        Name = "ITEM WITH NO NAME";
    }

    public virtual void Use ( Player player ) {
        // Nothing here?
    }
}

public abstract class Food : Item {
    public float Energy { get; set; }

    public Food () {
        Name = "FOOD WITH NO NAME";
        Energy = 1.0f;
    }

    public override void Use ( Player player ) {
        player.Energy += Energy;
    }
}

public class Meat : Food {
    public Meat () {
        Name = "Meat";
        Energy = 10.0f;
    }
}

Does this at all make sense?

\$\endgroup\$
4
\$\begingroup\$

I think your design introduces a few more dependencies than it should.

Let's have a look at your IUsable interface

public interface IUsable<Player> {
    void Use ( Player player );
}

Do you really need to have a dependency on Player? Why don't you just require it to depend on IUser?

I'd also completely get rid of the Item class. What do you need it for? If you want an abstract class to represent a NamedItem you can create it but you should do it in the following way:

public abstract class NamedItem implements IUsable<IUser>
{
    public string Name {get; private set;} // Better to keep it immutable

    protected NamedItem(string name)
    {
        Name = name;
    }

    public abstract void Use(IUser user);
    /*
     * This way we don't need to provide a 
     * default implementation and your subclasses
     * will be forced to provide it.
     */
}

I think you should probably also revise who should be in charge of implementing the Use method. From the names you're using it is quite clear that it is more natural to think at a player that uses an item. That's fine but what your code suggests is that different objects should be used in different ways, hence you need to implement it on the specific Item instance.

Does spreading the logic on how a player uses an item across a lot of different items makes sense in your project? If you don't want to do it you might want to have a look at dobule dispatch as a possible way to retain all the behaviours on player instead than on objects. If you do double dispatch you can have interfaces such as IFood and then have a method in Player with this signature void User(IFood food). IFood should expose a way to determine how many energy is added by the food to the player and the mutation of the state of the player should happen in the specific method you just created.

If you show us how you intend to use those classes we can help you find the best way to design them.

\$\endgroup\$
  • 1
    \$\begingroup\$ implements is a Java-ism :) \$\endgroup\$ – ChrisWue Nov 2 '14 at 18:51
  • \$\begingroup\$ Whenever the player clicks on an item in the inventory, it should de "used" by the player. How it's used in relation to the player differs, of course; items which derives from the Food class, should be eaten, while weapons should be equipped. My problem is to understand which logic should go where, i.e. when an item is clicked, should the Player object use the item, or should the Item object be used by the player, ref. "Player.Use(item)" vs "Item.Use(player)", or should I approach it totally differently? \$\endgroup\$ – toreau Nov 3 '14 at 13:43
1
\$\begingroup\$

You have some naming issues. Namely this I find very confusing:

public interface IUser<Item> {
    void Use ( Item usableItem );
}

Since you also have an class Item this is a bit distracting. General C# naming convention is to prefix generic type parameters with T. So better would be:

public interface IUser<TItem>
{
    void Use(TItem usableItem);
}

or

public interface IUser<T> 
{
    void Use(T usableItem);
}

Another standard convention in C# is to place opening braces in their own line (at least for classes, interfaces and methods) as shown above although admittedly there are some exceptions to that (mostly for auto properties, simple get/set properties and guard clauses).

An advantage of sticking to standard language ecosystem coding conventions is that it makes it easier for other programmers to read your code.

\$\endgroup\$

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.