2

I have several classes that all inherit from the same Shape class. When I create a new shape I want it to be of a random shape. The way that I thought to do it is to create a list that will hold links to all the constructors, and when I need to create a new shape I'll get a random constructor from the list and use it to construct my shape. I've tried to create the list in the fallowing way, but I get errors:

List<Action> constList = new List<Action>();
constList.Add(SShape());
constList.Add(OShape());
constList.Add(LShape());

The Shape constructor is defined as:

class Shape
{
    public Shape(PlayGrid grid, Color color)
    {
        ...
    }
    ...
}

And each sub shape's constructor is defined like:

class IShape : Shape
{
    public IShape(PlayGrid grid, Color color) : base(grid, color)
    {
    ...
    }
...
}

What is the correct way to construct the list, and what is the way to use the constructors from the list?

The contractors also need to get parameters that change between different shapes.

SIMEL
  • 8,745
  • 28
  • 84
  • 130
  • 1
    In your shape base class, have a static function, GetRandomShape() – Mike C. Mar 22 '13 at 12:24
  • 1
    Are your shape classes subclasses of Action? – Stealth Rabbi Mar 22 '13 at 12:24
  • @MikeC. the most un-OOP solution.... **Edit:** actually it depends on how it's being implemented. – gdoron Mar 22 '13 at 12:25
  • @MikeC., how do I do it, how can I make the class `Shape` return a son object? – SIMEL Mar 22 '13 at 12:26
  • Typically I'd use a Factory, like floAr below has an answer for, if this is for a 'real' application, and as gdoron mentions, you want to follow better OOP practice, factory is more typical. I disagree though that having a static method on your base class violates OOP, but I don't stare at OOP principles all day either. – Mike C. Mar 22 '13 at 12:35
  • What makes a Factory a bad idea in an OOP environment? Variations of Factories are very useful, and imo don't violate OOP at all. – Øyvind Bråthen Mar 22 '13 at 12:42
  • 1
    I believe what @gdoron was getting at was that it's not good OOP for a base class to have hardcoded reference to its subclasses, because it creates a dependency cycle. – MattW Mar 22 '13 at 12:45

6 Answers6

5

This concept can work, you just have the syntax for generating a delegate wrong, and you want a Func<PlayGrid, Color, Shape> not an Action:

var constList = new List<Func<PlayGrid, Color, Shape>>();
constList.Add((pg, c) => new SShape(pg, c));

PlayGrid playgrid = /* however you get this */;
Color color = /* however you get this */;
var shape = constList[randomIdx](playgrid, color);
MattW
  • 4,480
  • 1
  • 12
  • 11
  • And how do I call the contractor when I want to create a new object? – SIMEL Mar 22 '13 at 12:27
  • @IlyaMelamed Answer updated to account for the fact that yes, you do care about the object returned by the constructor. – MattW Mar 22 '13 at 12:32
  • The constractor takes parameters, and I can't add `constList.Add(() => new SShape());` to the list because it demands that I put in parameters. However, the parameters need to change each time I create an object so I can't put the parameters when I add the constructor to the list. – SIMEL Mar 23 '13 at 18:34
  • Does the number and type of parameters required vary between shapes, or is it the same for all? If it varies, you're going to need a new approach; if it's the same, you can just change the flavor of `Func<>` you use, for example, if they all take only the `PlayGrid` and `Color` you mention in the updated question, my updated answer will deal with it. – MattW Mar 23 '13 at 18:40
  • they all take the same parameters. As shown in the updated question. – SIMEL Mar 23 '13 at 18:45
2

You could do something like this:

public class ShapeFactory()
{
   //list of shape constructor functions
   private List<Func<Shape>> constructors;

   //static initalizaton of the constructor list
   static ShapeFactory()
   {
      constructors = new List<Func<Shape>>();
      constructors.Add( () => new OShape());
      constructors.Add( () => new SShape());
      constructors.Add( () => new LShape());
      ....
   }

   public static Shape CreateRandomShape()
   {
       int index = ...get random index somehow...
       return constructors[index]();
   }
}

and use it in code with

Shape randomShape = ShapeFactory.CreateRandomShape();
SWeko
  • 30,434
  • 10
  • 71
  • 106
0

I believe you want to create an Interface (perhaps 'IShape')for all shapes, and then use a factory to instantiate them (at which time you could randomly instantiate any type that implements your IShape interface.)

Frank Thomas
  • 2,434
  • 1
  • 17
  • 28
0

The responsibility for creating a random object in this way should not be a behavior of the object itself. I suggest you create a factory to manage this. Your factory can generate the list of constructor calls (or pre-cache the objects) and then provide one of them in its create method. Your factory should return constructed objects (probably as an Interface, like IShape), and not Actions to invoke to create the object. That way, if your factory needs to inject dependencies or set other values, it can do so and manage the construction of the objects.

Also, if that is the .NET Action class, I'm guessing your Shape classes do not inherit from it, but it's unclear from your question what "I get errors" is.

THere is a comment about having a GetRandomShape() method in your base class. You do not want this, because then your base class knows about, and is dependent on, its subclasses, which violates OO Design practices.

Stealth Rabbi
  • 10,156
  • 22
  • 100
  • 176
0

You may save the type of the classes to a list and use Activator.CreateInstance(Type) to optain a new instance.

Or you create a list of type Shape, add one instance of every class and use the .Clone() method to obtain a new instance.

Or you implement a factory pattern.

floAr
  • 799
  • 1
  • 6
  • 29
0

This is a continuation of what you started. It uses Reflection to invoke the constructors. The initial list is a list of types, not of Actions.

class Program
    {
        static void Main(string[] args)
        {


            //register the types
            List<Type> types = new List<Type>() { typeof(OShape), typeof(PShape), typeof(RShape) };

            // randomly select one
            Type type = types[1];

            // invoke the ctor
            ConstructorInfo ctor = type.GetConstructor(/*optional param for ctor, none in this case*/ new Type[] {} );
            object instance = ctor.Invoke(new object[] {});

            // you can safely cast to Shape
            Shape shape = (Shape)instance; //this is a PShape!

        }
    }

    class Shape
    {
    }

    class OShape : Shape
    {
    }

    class PShape : Shape
    {
    }

    class RShape : Shape
    {
    }
}

The next thing to think about is how to allow subtypes of Shape to receive parameters. You'd need to a factory pattern, i.e. an object that knoes how to construct other object and will do so based on - for example - a string:

Shape s = Factory.GetShape("OShape", new[] object {4});

Check this question out.

Community
  • 1
  • 1
Bogdan Gavril MSFT
  • 20,615
  • 10
  • 53
  • 74
  • 1
    The way you've done this, apart from making it work with C# compilers before version 3, I see no benefit in using reflection. You haven't eliminated hard references to the shapes, so your reflection is simply doing exactly what the delegate-based approach the question was attempting does when it's done right, only _much_ slower. If you'd brought in something like [MEF](http://msdn.microsoft.com/en-us/library/dd460648.aspx) to remove the dependency on the implementations of Shape it would make more sense to me. – MattW Mar 22 '13 at 13:01
  • 1
    +1 for mentioning MEF :) Actually there is one advantage to my solution - it supports ctors with parameters, unlike all the other answers that rely on Func. – Bogdan Gavril MSFT Mar 23 '13 at 18:24