Getting past Open-Closed Principle

Question

I have a simple program which draws geometrical figures based on mouse data provided by user. I've got one class which handles the mouse tracking (it gets the List with mouse movement history) and one abstract class called Shape. From this class I derieve some extra Shapes, like Circle, Rectangle, etc. - and every one of them overrides the abstract Draw() function.

It all works nicely, but the problem comes when I want the user to be able to switch desired Shape manually. I get the mouse data and i know what shape should I draw. The problem is how to make the code to "know" which object should it create and pass appropiate parameters to the constructor. It is also impossible at this point to add new Shape derivatives, which is obiously wrong.

I obiously don't want to come out with code like:

List<Shape> Shapes = new List<Shape>();
// somwhere later 

if(CurrentShape == "polyline"){
    Shapes.Add(new Polyline(Points)); 
}
else if (CurrentShape == "rectangle"){
    Shapes.Add(new Rectangle(BeginPoint, EndPoint));
}
// and so on.

The code above clearly vilates the Open-Closed Principle. The problem is that I don't have any good idea how to get over it. The main problem is that different Shapes have constructors with different parameters, which makes it much more troublesome.

I am pretty sure that this is a common problem, but I don't know how to get past it. Do you have ay idea?

Bribery

That's not the "Open-Closed Principle". That's just polymorphism – Thorax

Well, I want the Shape class code to be closed for edition and opened for extensions, so I think that it matches the OCP problem. – Bribery

As much as you'd like it to be true, it's not! – Thorax

You need to Google for Factory pattern. – Pali

OK, so even if it is only a polymorphism problem, I still can't come with a sollution to the problem. – Bribery

I disagree Mitch, ok it's not the Shape class that is violating the principle, but the consumption code above. And yes, you would solve this with factories. – Great

@MitchWheat No, it's not just polymorphism: creation of objects is not polymorphic in C#. There's some work to be done to implement it right, which does involve some polymorphism, but of a different set of objects. – Belton

In this specific case, both classes accept a list of points so you could have a common Shape constructor with a list of points, and create the class instance using the name and a bit of reflection. – Intoxicating

Yep, polymorphism and factory pattern – Thorax

...to solve a violation of the open/closed principle – Great

Simon - I don't want to always pass the List as an parameter, because f.e. Rectangle needs only 2 Point objects (last and first in the list) and I think it would be a bad sollution to give it f.e. 1000 points of mouse tracking, only to get rid of 998 of them the moment I get into constructor. – Bribery

I never said that. Two points are still a list of points, so the base constructor could be Shape(IEnumerable<Point> points). It's up to the rectangle overload to check only two points are provided. Actually, dasblinkenlight provides the same idea in his answer. – Intoxicating

OK, so you're violating the Open-Closed principle. Is that principle even relevant to your customers? Are you going to lose any sales because your customers say that you're violating the OCP? Is being open to extension necessary? I violate the OCP all the time because complying with the OCP frequently adds costs without adding corresponding value. Worry about problems that actually impact customers. – Madore

Eric Lippert - sorry for being such a Slowpoke, I've just read Your comment after a month... I know that a problem is not important for customers, but it is a university project ;) So I have to (or, at least, I want to) do my best not to violate the principles :) – Bribery

Answer 1

It begs for a factory but not just the factory but factory with injectable workers.

public class Context {
   public Point BeginPoint;
   public Point EndPoint;
   public List Points;

   whatever else
}

public class ShapeFactory {

   List<FactoryWorker> workers;

   public Shape CreateShape( string ShapeName, Context context )
   {
      foreach ( FactoryWorker worker in workers )
         if ( worker.Accepts( ShapeName ) )
             return worker.CreateShape( context );
   }

   public void AddWorker( FactoryWorker worker ) {
      workers.Add( worker );
   }
 }

 public abstract class FactortWorker {
    public abstract bool Accepts( string ShapeName );
    puboic Shape CreateShape( Context context );
 }

 public class PolyLineFactoryWorker : FactoryWorker {

    public override bool Accepts( string ShapeName ) {
       return ShapeName == "polyline";
    }

    public Shape CreateShape( Context context ) { ... }

 }

This way the code is open for extensions - new factory workers can be created freely and added to the factory.

Chiffon

Hi. I'd like to know, how is this better than the other answer extending ShapeFactory instead of injecting workers, if the other answer used a string ShapeName parameter as well? – Tabasco

@Blueriver: This would make both answers equivalent. But, there is no factory extending in the other answer. Makers from there are badically workers from here. The factory itself should never be modified as it would break the Closed part od the OCP. – Chiffon

Answer 2

When you need to create objects that all derive from a single class or implement the same interface, one common approach is to use a factory. In your case, however, a simple factory may not be sufficient, because the factory itself needs to be extensible.

One way to implement it is as follows:

interface IShapeMaker {
    IShape Make(IList<Point> points);
}
class RectMaker : IShapeMaker {
    public Make(IList<Point> points) {
        // Check if the points are good to make a rectangle
        ...
        if (pointsAreGoodForRectangle) {
            return new Rectangle(...);
        }
        return null; // Cannot make a rectangle
    }
}
class PolylineMaker : IShapeMaker {
    public Make(IList<Point> points) {
        // Check if the points are good to make a polyline
        ...
        if (pointsAreGoodForPolyline) {
            return new Polyline(...);
        }
        return null; // Cannot make a polyline
    }
}

With these Maker classes in hand, you can make a registry of makers (a simple List<IShapeMaker>) go through the makers passing them the points, and stopping when you get back a non-null shape.

This system remains extensible, because you can add a pair of NewShape and NewShapeMaker, and "plug them in" into the existing framework: once NewShapeMaker gets in the registry, the rest of the system instantly becomes ready to recognize and use your NewShape.

Belton

There is a drawback of this implementation - since the same set of points could possibly be accepted by multiple makers, suddenly the order of makers matters - changing the order would make the factory return a different shape. The original implementation is free of this issue, as the factory client makes it explicit which shape it wants. This would easily be fixed by passing this additional parameter to the factory method (see my answer). – Chiffon

@WiktorZychla The extra parameter gives explicit control to the caller, but it also makes it necessary to modify the caller each time that you extend the system by adding a new shape (i.e. the caller needs to "learn" to pass the name of the new shape each time a new shape become available). My reading of the question is that the OP wants to avoid that extra parameter. – Belton

He writes "I know the mouse data and the shape I should draw". From this, I would say the caller knows the exact shape. I believe he has a toolbox of all available shapes or something similar. – Chiffon

@WiktorZychla - You are right, the user has a toolbar with shapes (and colors, which is irrelevant to the problem itself). It turns out that Your answer is a slightly better sollution to my problem. – Bribery

Hi. I'd like to know, how is this worse than the other answer injecting workers instead of extending ShapeFactory, if this answer used a string ShapeName parameter as well? – Tabasco