1

This question may be a bit subjective, but I have tried three different solutions and none of them has felt right. I will provide some context and the solutions I have tried. The issue I am facing seems to boil down to whether or not I should split an interface into multiple interfaces / sub-interfaces, or maybe make it generic instead. Three different solutions (that I am aware of).

I am working on an inventory system for a game. Items, which can be placed in the inventory, can have an action associated with them which will be performed when the item is clicked. This is accomplished through the use of an interface, IAction, which has a void Perform(IActor actor) method declaration. Not all items implement IAction, and different items may have different actions to be performed, causing the IActor interface to contain a lot of method declarations.

The first solution I used had all these methods on IActor and I did not like it since not all actors needed all the methods on IActor. I decided to create sub-interfaces of IActor, such as IHealthUser, as a way to segregate the methods. The reason I kept IActoras a base interface was because of the void Perform(IActor actor)method declaration on IAction. The solution ended up looking like this:

public interface IActor { ... }
public interface IHealthUser : IActor { void AddHealth(int amount); }
public interface IAction { bool Perform(IActor actor); }
public class HealthPotion : IAction
{
    public bool Perform(IActor actor)
    {
        var performedAction = false;
        var healthUser = actor as IHealthUser;
        if (healthUser != null)
        {
            healthUser.AddHealth(Amount);
            performedAction = true;
        }
        return performedAction;
    }
}

and is called from the event handler method in the inventory like this

var action = Item as IAction;
if (action != null)
{
    var performedAction = action.Perform(Owner); // Where 'Owner' is IActor
    ...
}

I had to change the return type of Perform() from void to bool to indicate whether or not the action was actually performed, since I have to cast actor inside the method before being able to do perform the action. This works, but I am not sure it is a good / "standard" solution.

I also tried making IAction generic:

public interface IAction<T> where T : IActor { void Perform(T actor); }

which makes the implementation of IAction<T> quite clean and also gets rid of the bool return value:

public class HealthPotion : IAction<IHealthUser>
{
    public void Perform(IHealthUser healthUser)
    {
        healthUser.AddHealth(Amount);
    }
}

This solution, however, puts a lot of burden on the callers, which I do not have a good solution for:

// Do we have to cast and test for every possible IActor interface?
var action = Item as IAction<IHealthUser>;
var action = Item as IAction<IManaUser>;
...

The easiest solution is to just let all methods stay on IActor, but that is ugly. The current solution, casting inside Perform() works ok. The generic solution feels most right of the three, if it were not for the issue I have with figuring out what to cast Item to before calling Perform(). My question, therefore, is, is the generic solution the way to go and if so, how should I go about figuring out what to cast Item to?

  • Your first option with all the methods in one interface is a fine example of visitor pattern. Do not split and cast, that corrupts typesafety. – Basilevs Jan 24 '17 at 1:25
  • Which version of C# are you using? – David Arno Jan 24 '17 at 8:56
  • "Items ... can have an action associated with them" - that's a "has a" relationship, but you created an "is a" relationship. Also I'd expect the IActor ot perform an IAction with some object, not the object with an actor. – Bernhard Hiller Jan 24 '17 at 9:37
  • @DavidArno I believe it is C# 3.0. – user1323245 Jan 24 '17 at 12:47
  • If you're passing everything around as IActor and IAction then someone somewhere has to cast the objects to their real types. There is just no way around it. The only solution is design your model in a way that doesn't require everything to be passed around as base interfaces. – 17 of 26 Jan 24 '17 at 17:07
-2

Use Adapter pattern to isolate Visitor's clients from unnecessary actions.

Implement all methods of IActor an simple implementation class, making all method bodies empty. Make every client create an instance of IActor overriding necessary methods of implementation class. This will make clients independent of the action they do not support and will keep advantages of classical Visitor. Do not make clients implement IActor themselves, this will introduce insurance internal dependencies.

When the question sounds "what do I cast to" always ask "Why do I need a cast, what has gone wrong?" instead. Look for a solution expressed in type system first, do not drop it right away.

public interface IActor {
   void addHealth(int amount);
   void addMana(int amount);
}

public class ActorAdapter: IActor {
   void addHealth(int amount) {}
   void addMana(int amount) {}
}

class DullBeast {
   private int health = 40;
   public void getActor() {
      return new ActorAdapter() {
         void addHealth(int amount) {
            health += amount;
         }
         //I know nothing about magic and mana
      };
   }
}

class Magician {
   private int health = 60;
   private int mana = 10;
   public void getActor() {
      return new ActorAdapter() {
         void addHealth(int amount) {
            health += amount;
         }
         void addMana(int amount) {
            mana += amount;
         }
      };
   }
}

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.