4

I created a little abstract domain to illustrate the problem I am facing, so there it is.

There is a medieval game, where the players are generals of their army and the entire battle is mostly affected by the battle plan, which is made before the battle begins, in let's say preparation mode.

To achieve what's needed, I created an interface IBattleUnit and kept things pretty simple:

public interface IBattleUnit
{
    void Move();
    void Attack();
    string Salute();
}

Having three types of units will do the job for now, so Archer.cs, Pikeman.cs and Swordsman.cs implement the interface in pretty much the same way:

public class Swordsman : IBattleUnit
{
    private Swordsman() {}

    public void Move()
    {
        //swordsman moves
    }

    public void Attack()
    {
        //swordsman attacks
    }

    public string Salute()
    {
        return "Swordsman at your service, master.";
    }
}

Note the private constructor, it is intended for battle units to be recruited only in Barracks, this is the generic factory

public static class Barracks<T> where T : class, IBattleUnit
{
    private static readonly Func<T> UnitTemplate = Expression.Lambda<Func<T>>(
       Expression.New(typeof(T)), null).Compile();

    public static T Recruit()
    {
        return UnitTemplate();
    }
}

Note: precompiled lambda expressions for the empty constructor make (on my machine) unit creation faster, and whereas the army can get really big, fast generic creation is exactly what I want to achieve.

For having covered everything a battle needs to be started, the BattlePlan explanation is the only missing part, so here we come:

public static class BattlePlan
{
    private static List<Type> _battleUnitTypes;
    private static List<Type> _otherInterfaceImplementors;
    //...

    private static Dictionary<string, string> _battlePlanPreferences;

    private static Type _preferedBattleUnit;
    private static Type _preferedTransportationUnit;
    //...

    static BattlePlan()
    {
        //read the battle plan from file (or whereever the plan init data originate from)
        //explore assemblies for interface implementors of all kinds
        //and finally fill in all fields
        _preferedBattleUnit = typeof (Archer);
    }

    public static Type PreferedBattleUnit
    {
        get
        {
            return _preferedBattleUnit;
        }
    }

    //... and so on

}

Now if you have reached this, you are aware of the whole domain - it even compiles and everything looks bright, until...

Until now: I create a console application, add references to the above mentioned, and try to profit from what's under the hood. For complete description of my confusion, I note what IS WORKING first:

  • If I want the Barracks to give me a specific BattleUnit, I can instantiate it and let it fight, move and salute. If the instantiation is done this way:
IBattleUnit unit = Barracks<Pikeman>.Recruit();
  • If I want to know what is the prefered unit based on battle plan, I can get it, I can ask for its AssemblyQualifiedName, I get the Type (in fact it is Archer, just as it stays in BattlePlan) , long story short, I get what I expect to, when I call:
Type preferedType = BattlePlan.PreferedBattleUnit;

And here, when I expect the BattlePlan to supply me with a Type and me just passing the Type to Barracks in order to instantiate some kind of Unit, VisualStudio2012 (resharper of current version) stops me and does not compile the code, while the code, that leads to the error is:

Type t = Type.GetType(BattlePlan.PreferedBattleUnit.AssemblyQualifiedName);
IBattleUnit u = Barracks<t>.Recruit();

No matter what I do, no matter whether I pass the t, or pass it as typeof(t), or try converting it to IRepository ... I still end up not being able to compile such code, with (at least) two errors in the error list:

Error   1   Cannot implicitly convert type 't' to 'BattleUnits.cs.IBattleUnit' Program.cs
Error   2   The type or namespace name 't' could not be found (are you missing a using directive or an assembly reference?) Program.cs

So to the actual questions:

  1. Is there some way, I could pass the type to Barracks, not having to change underlying infrastructure?
  2. Or is there anything I am doing wrong by design?

I have spent the last two days googling around and still, with the only clear way being changing the Barracks, which in fact is what I would not want to.

EDIT no.1: When re-thinking the concept and everything : IBattleUnit was first described as a set of core battle actions every Unit will be able to do (and we want it to be this way). I did not want to introduce base classes, just because I knew, there could possibly be GroundUnitBase and FlyingUnitBase abstract classes for the sake, we would like to have clear and logical design... But there absolutely has to be only one static Barracks.

Still for the BattleUnits - putting one base class in my eyes now seems could change the things for code being runnable and I'm right on my way of trying that out ... reading, what I wrote made me think about UnitBase class could possibly help not even the design but in some way its compilability. So this is the first idea in my mind after rethinking what's written.

halfer
  • 19,824
  • 17
  • 99
  • 186
jmodrak
  • 229
  • 4
  • 17
  • The basic issue is that generic parameters have to be constant at compile time. I might suggest, however, that you change the nature of your interfaces to ISalute, IAttack, and IMove, thereby allowing a concrete warrior that has the particular features you want. That'll get you further from the problem you're having, as well as base classes. – Magus Nov 14 '13 at 22:24

5 Answers5

1
public static class Barracks
{
    public static IBattleUnit Recruit(Type preferredType)
    {
        return (IBattleUnit)typeof(Barracks<>).MakeGenericType(preferredType).GetMethod("Recruit", BindingFlags.Public|BindingFlags.Static).Invoke(null,null);
    }
}

then call

Barracks.Recruit(BattlePlan.PreferredBattleUnit)
smartcaveman
  • 41,281
  • 29
  • 127
  • 212
  • Thanks, this does the job done! Instantiates an archer, if BattlePlan preferes archers, and an pikeman, if we go for Pikeman. Still, it needs little redesign: either separate class that has your Recruit method which will call my Recruit method with precompiled lambdas, or I will just put my Barracks class as nested class inside yours. More to my concern, I am heading to validate performance effects of this approach, while they do not seem to be too much of issue from the first perspective. – jmodrak Nov 14 '13 at 22:40
  • @jmodrak - have you heard of inversion of control? There are lots of frameworks that do this kind of thing for you in a way more flexible way. Look up StructureMap, Castle Windsor, Ninject, etc. There's actually a video on it with a strikingly similar domain model to yours : http://www.youtube.com/watch?v=dEu__OKeL6A – smartcaveman Nov 14 '13 at 22:55
  • I'm actually familiar with WindsorCastle, but IoC is for some reasons not appliable here – jmodrak Nov 14 '13 at 23:03
  • I benchmarked this approach on creating 10M instances against the original and the performance difference is just insane; when using the previous - that means without the .MakeGeneric and .GetMethod you get it done in less than a second, while your approach consumes approximately 15seconds on my machine. That is clearly not as fast as I hoped for, will continue tweaking on that. But every additional piece of info is warmly welcome. – jmodrak Nov 15 '13 at 01:41
  • @jmodrak - if you are evaluating the results for performance, then you should cache the reflection results in a `IDictionary`. You could even take this further and build the delegate, and have an `IDictionary>`. Try bench marking it after that. The point of my answer was to teach you how you can call the method that was generating the error, not to provide you with production ready code. – smartcaveman Nov 18 '13 at 09:06
  • Agreed, smartcaveman, thanks for help. The solution, I am currently using is from the most part a mixture of all valuable answers in this topic, inlcuding yours. But still, performance was my biggest concern, as stated in the question from the begining. And there the thing I did not see at all is, I do not really need `Barracks`to be generic. – jmodrak Nov 18 '13 at 18:51
1

You can do this using reflection, something like this:

IBattleUnit unit = typeof(Barracks).GetMethod("Recruit").MakeGenericType(BattlePlan.PreferedBattleUnit).Invoke(null, null) as IBattleUnit;
mayabelle
  • 9,804
  • 9
  • 36
  • 59
1

If you have an instance of the PreferedBattleUnit you simply need to use the dynamic keyword. Please have a look at this question (John Skeet answer): (EDIT: This might not be very helpful as your method is not generic)

Pass concrete object type as parameter for generic method

If you don't have an instance of the object than have a look at the following question (again, John Skeet answer):

Generics in C#, using type of a variable as parameter

Community
  • 1
  • 1
Moslem Ben Dhaou
  • 6,897
  • 8
  • 62
  • 93
1

You don't really need Barracks to be generic. This solution doesn't use reflection so it's much more efficient:

public static class Barracks
{
    private static readonly IDictionary<Type, Func<IBattleUnit>> FactoryMethods = new Dictionary<Type, Func<IBattleUnit>>();

    public static void Register<T>(Func<IBattleUnit> factory) where T : IBattleUnit
    {
        FactoryMethods.Add(typeof(T), factory);
    }

    public static IBattleUnit Recruit<T>() where T : IBattleUnit
    {
        return Recruit(typeof (T));
    }

    public static IBattleUnit Recruit(Type type)
    {
        Func<IBattleUnit> createBattleUnit;
        if (FactoryMethods.TryGetValue(type, out createBattleUnit))
        {
            return createBattleUnit();
        }

        throw new ArgumentException();
    }
}

public class Swordsman : IBattleUnit
{
    static Swordsman()
    {
        Barracks.Register<Swordsman>(() => new Swordsman());
    }
}
Leonardo
  • 2,065
  • 2
  • 26
  • 27
  • Took me a while to catch up with your post and I am definitelly on my way of trying it. But my question is, what would happen, if the Swordsman class does not know about Barracks : Barracks reside in different project and Swordsman does not have the reference to Barracks? – jmodrak Nov 14 '13 at 23:08
  • @jmodrak: That's a very good point. This is essentially a service locator hidden inside a static class constructor, which is remarkably dangerous. – Magus Nov 14 '13 at 23:39
  • @ldalonzo: but as I see it, it could be possible to register all types I want to register in Barracks' static constructor. I need the precompiled lambda constructors, they proven to be extremely efficient in creating large amount of objects, that are initially identic (parameterless constructors) and atm I am evaluating on between your and smartcaveman's answer, where production will see a mixture, with drive towards performance. – jmodrak Nov 14 '13 at 23:45
  • It's a hidden dependency no matter where you put the declaration. – Magus Nov 14 '13 at 23:49
  • @Alex Beisley: not necessarilly "no matter". Barracks need to know about the types of soldiers. But why would soldiers need to know, there are barracks? So I would not agree on the no matter term. – jmodrak Nov 14 '13 at 23:56
  • @jmodrak: From outside Barracks, it isn't obvious what it contains. If something hasn't been added to it and you try to access it, you get an exception at runtime. Within either object is definitely the wrong place to register these, but even if it's outside you have to configure a static class, which means your code will only work in a specific configuration. [Why this is a bad idea](http://blog.ploeh.dk/2010/02/03/ServiceLocatorisanAnti-Pattern/) – Magus Nov 15 '13 at 00:16
  • @Alex Beisley: Thanks for the link! But hey, this is why we have a BattlePlan => it explores assemblies for implementors and fills the fields. And what I forgot to take in, there are default values, that come into place, if the configuration file (or stream) does not exist (or contain reasonable data). But I would feel incomfortable, If i had to keep those info and initialization anywhere else than in BattlePlan (so please excuse my mistake -> here comes the correction: I will Register object types into Barracks from BattlePlan). – jmodrak Nov 15 '13 at 00:25
  • @jmodrak: If your code will throw runtime exceptions if code somewhere else is changed, it is not well designed. If you were to copy parts of the code into a new project and forget others or run the code in a unit test, the code would fail. It requires you to set up a fake configuration within the test, which shouldn't be necessary for something this small, and it won't be obvious what went wrong or how to fix it. How you organize a bad design is irrelevant. – Magus Nov 15 '13 at 16:50
  • @Alex Beisley: Ok, what you suggest is to keep the configuration within Barracks, because thats where it belongs and again, thats the only place where it is used. Well, that sounds reasonable enough. – jmodrak Nov 15 '13 at 16:52
  • @jmodrak - you could use caching with my approach and wouldn't need to expose or manually configure the registry. Alex Beisley makes some good points that you should consider as to why you shouldn't use this. – smartcaveman Nov 18 '13 at 09:12
0

My strategy would be to create a Dictionary<Type, Barracks<IBattleUnit>>, assuming you intend to have all the barracks defined before you try to retrieve from them. That way you can match by the key and cast safely.

This would require the Barracks<> to not be a static class. Unless you have very specific reasons like some kind of external resource you're managing (and arguably even then), you probably have no need for a static class.

While it may seem like creating statics for all of these will make everything easier, ultimately you create a dependency on a resource that may change. If you invent another unit type, you have to register it with the barracks, which is in no real way different than the reason you don't want to make base classes, and if you forget you'll throw exceptions, which is even worse, because it violates the Principle of Least Surprise.

Magus
  • 1,302
  • 9
  • 16