9

After doing some researches I can not seem to find a simple example resolving a problem I encounter often.

Let's say I want to create a little application where I can create Squares, Circles, and other shapes, display them on a screen, modify their properties after selecting them, and then compute all of their perimeters.

I would do the model class like this:

class AbstractShape
{
public :
    typedef enum{
        SQUARE = 0,
        CIRCLE,
    } SHAPE_TYPE;

    AbstractShape(SHAPE_TYPE type):m_type(type){}
    virtual ~AbstractShape();

    virtual float computePerimeter() const = 0;

    SHAPE_TYPE getType() const{return m_type;}
protected :
    const SHAPE_TYPE  m_type;
};

class Square : public AbstractShape
{
public:
    Square():AbstractShape(SQUARE){}
    ~Square();

    void setWidth(float w){m_width = w;}
    float getWidth() const{return m_width;}

    float computePerimeter() const{
        return m_width*4;
    }

private :
    float m_width;
};

class Circle : public AbstractShape
{
public:
    Circle():AbstractShape(CIRCLE){}
    ~Circle();

    void setRadius(float w){m_radius = w;}
    float getRadius() const{return m_radius;}

    float computePerimeter() const{
        return 2*M_PI*m_radius;
    }

private :
    float m_radius;
};

(Imagine I have more classes of shapes: triangles, hexagones, with each time their proprers variables and associated getters and setters. The problems I faced had 8 subclasses but for the sake of the example I stopped at 2)

I now have a ShapeManager, instantiating and storing all the shapes in an array :

class ShapeManager
{
public:
    ShapeManager();
    ~ShapeManager();

    void addShape(AbstractShape* shape){
        m_shapes.push_back(shape);
    }

    float computeShapePerimeter(int shapeIndex){
        return m_shapes[shapeIndex]->computePerimeter();
    }


private :
    std::vector<AbstractShape*> m_shapes;
};

Finally, I have a view with spinboxes to change each parameter for each type of shape. For example, when I select a square on the screen, the parameter widget only displays Square-related parameters (thanks to AbstractShape::getType()) and proposes to change the width of the square. To do that I need a function allowing me to modify the width in ShapeManager, and this is how I do it:

void ShapeManager::changeSquareWidth(int shapeIndex, float width){
   Square* square = dynamic_cast<Square*>(m_shapes[shapeIndex]);
   assert(square);
   square->setWidth(width);
}

Is there a better design avoiding me to use the dynamic_cast and to implement a getter/setter couple in ShapeManager for each subclass variables I may have? I already tried to use template but failed.


The problem I'm facing is not really with Shapes but with different Jobs for a 3D printer (ex: PrintPatternInZoneJob, TakePhotoOfZone, etc.) with AbstractJob as their base class. The virtual method is execute() and not getPerimeter(). The only time I need to use concrete usage is to fill the specific information a job needs :

  • PrintPatternInZone needs the list of points to print, the position of the zone, some printing parameters like the temperature

  • TakePhotoOfZone needs what zone to take into photo, the path where the photo will be saved, the dimensions, etc...

When I will then call execute(), the Jobs will use the specific information they have to realise the action they are supposed to do.

The only time I need to use the concrete type of a Job is when I fill or display theses informations (if a TakePhotoOfZone Job is selected, a widget displaying and modifying the zone, path, and dimensions parameters will be shown).

The Jobs are then put into a list of Jobs which take the first job, executes it (by calling AbstractJob::execute()), the goes to the next, on and on until the end of the list. (This is why I use inheritance).

To store the different types of parameters I use a JsonObject:

  • advantages : same structure for any job, no dynamic_cast when setting or reading parameters

  • problem : can't store pointers (to Pattern or Zone)

Do you thing there is a better way of storing data?

Then how would you store the concrete type of the Job to use it when I have to modify the specific parameters of that type? JobManager only has a list of AbstractJob*.

  • 5
    Seems like your ShapeManager will become a God class, because it will basically contain all setter methods for all types of shapes. – Emerson Cardoso Jan 4 '18 at 11:33
  • Have you considered a "property bag" design? Such as changeValue(int shapeIndex, PropertyKey propkey, double numericalValue) where PropertyKey can be an enum or a string, and "Width" (which signifies that the call to the setter will update the value of width) is among one of the allowed values. – rwong Jan 4 '18 at 11:35
  • Even though property bag is considered OO anti-pattern by some, there are situations when using property bag simplifies design, where every other alternative will make things more complicated. Though, to determine whether property bag is suitable for your use case, more information is needed (such as how to GUI code interacts with the getter/setter). – rwong Jan 4 '18 at 11:39
  • I considered the property bag design (even though I did not know its name) but with a JSON object container. It sure could work but I thought it was not an elegant design and that a better option may exist. Why is it considered an OO anti-pattern ? – ElevenJune Jan 4 '18 at 12:02
  • For example, if I want to store a pointer to use it later, how do I do ? – ElevenJune Jan 4 '18 at 12:10
10

I would like to expand on Emerson Cardoso's "other suggestion" because I believe it to be the correct approach in the general case - though you may of course find other solutions better suited to any particular problem.

The Problem

In your example, the AbstractShape class has a getType() method that basically identifies the concrete type. This is generally a sign that you don't have a good abstraction. The whole point of abstracting, after all, is not having to care about the details of the concrete type.

Also, in case you're not familiar with it, you should read up on the Open/Closed Principle. It's often explained with a shapes example, so you'll feel right at home.

Useful Abstractions

I assume you have introduced the AbstractShape because you found it useful for something. Most likely, some part of your application needs to know the perimeter of shapes, regardless of what the shape is.

This is the place where abstraction makes sense. Because this module does not concern itself with concrete shapes, it can depend on AbstractShape only. For the same reason, it does not need the getType() method - so you should get rid of it.

Other parts of the application will only work with a particular kind of shape, e.g. Rectangle. Those areas will not benefit from an AbstractShape class, so you shouldn't use it there. In order to pass only the correct shape to these parts, you need to store concrete shapes separately. (You may store them as AbstractShape additionally, or combine them on the fly).

Minimizing Concrete Usage

There is no way around it: you need the concrete types in some places - at the very least during construction. However, it's sometimes best to keep the use of concrete types limited to a few well-defined areas. These separate areas have the sole purpose of dealing with the different types - while all application logic is kept out of them.

How do you achieve this? Usually, by introducing more abstractions - which may or may not mirror the existing abstractions. For example, your GUI doesn't really need to know what kind of shape it is dealing with. It just needs to know that there is an area on the screen where the user can edit a shape.

So you define an abstract ShapeEditView for which you have RectangleEditView and CircleEditView implementations that hold the actual text boxes for width/height or radius.

In a first step, you could create a RectangleEditView whenever you create a Rectangle and then put it into a std::map<AbstractShape*, AbstractShapeView*>. If you would rather create the views as you need them, you might do the following instead:

std::map<AbstractShape*, std::function<AbstractShapeView*()>> viewFactories;
// ...
auto rect = new Rectangle();
// ...
auto viewFactory = [rect]() { return new RectangleEditView(rect); }
viewFactories[rect] = viewFactory;

Either way, the code outside of this creation logic will not have to deal with concrete shapes. As part of the destruction of a shape, you need to remove the factory, obviously. Of course, this example is over-simplified, but I hope the idea is clear.

Choosing the right Option

In very simple applications, you might find that a dirty (casting) solution just gives you the most bang for your buck.

Explicitly maintaining separate lists for each concrete type is probably the way to go if your application mainly deals with concrete shapes, but has some parts that are universal. Here, it makes sense to abstract only so far as the common functionality requires it.

Going all the way generally pays if you have a lot of logic that operates on shapes, and the exact kind of shape really is a detail to your application.

  • I really like your answer, you perfectly described the problem. The problem I'm facing is not really with Shapes but with different Jobs for a 3D printer (ex: PrintPatternInZoneJob, TakePhotoOfZone, etc.) with AbstractJob as their base class. The virtual method is execute() and not getPerimeter(). The only time I need to use concrete usage is to fill the specific information a job needs (list of points, position, temperature, etc) with a specific widget. Attaching a view to each job does not seems to be the thing to do in this particular case but I don't see how to adapt your vision to my pb. – ElevenJune Jan 10 '18 at 10:02
  • If you don't want to keep separate lists, you can use a viewSelector rather than a viewFactory: [rect, rectView]() { rectView.bind(rect); return rectView; }. By the way, this should of course be done in the presentation module, e.g. in a RectangleCreatedEventHandler. – doubleYou Jan 10 '18 at 10:52
  • 3
    This being said, try not to over-engineer this. The benefit of the abstraction must still outweigh the cost of the additional pluming. Sometimes a well placed cast, or separate logic may be preferable. – doubleYou Jan 10 '18 at 10:54
2

One approach would be to make stuff more general in order to avoid casting to specific types.

You could implement a basic getter/setter of "dimension" float properties in the base class, which sets a value in a map, based on a a specific key for the property name. Example below:

class AbstractShape
{
public :
    typedef enum{
        SQUARE = 0,
        CIRCLE,
    } SHAPE_TYPE;

    AbstractShape(SHAPE_TYPE type):m_type(type){}
    virtual ~AbstractShape();

    virtual float computePerimeter() const = 0;

    void setDimension(const std::string& name, float v){ m_dimensions[name] = v; }
    float getDimension() const{ return m_dimensions[name]; }

    SHAPE_TYPE getType() const{return m_type;}

protected :
    const SHAPE_TYPE  m_type;
    std::map<std::string, float> m_dimensions;
};

Then, in your manager class you need to implement only one function, like below:

void ShapeManager::changeShapeDimension(const int shapeIndex, const std::string& dimension, float value){
   m_shapes[shapeIndex]->setDimension(name, value);
}

Example of usage within the View:

ShapeManager shapeManager;

shapeManager.addShape(new Circle());
shapeManager.changeShapeDimension(0, "RADIUS", 5.678f);
float circlePerimeter = shapeManager.computeShapePerimeter(0);

shapeManager.addShape(new Square());
shapeManager.changeShapeDimension(1, "WIDTH", 2.345f);
float squarePerimeter = shapeManager.computeShapePerimeter(1);

Another suggestion:

Since your manager only exposes the setter and the perimeter computation (which are exposed by Shape as well), you could simply instantiate a proper View when you instantiate a specific Shape class. EG:

  • Instantiate a Square and a SquareEditView;
  • Pass the Square instance to the SquareEditView object;
  • (optional) Instead of having a ShapeManager, in your main view you could still keep a list of Shapes;
  • Within SquareEditView, you keep a reference to a Square; this would eliminate the need of casting for editing the objects.
  • I like the first suggestion and already thought about it, but it is quite limiting if you want to store different variables (float, pointers, arrays). For the second suggestion, if the square is already instantiated (I cliked on it on the view) how do I know it's a Square* object ? the list storing the shapes returns an AbstractShape*. – ElevenJune Jan 4 '18 at 12:17
  • @ElevenJune - yes all suggestions have their drawbacks; for the first you would need to implement something more complex rather than a simple map if you want more types of properties. The second suggestion changes how you store the shapes; you store the base shape in the list, but at the same time you need to provide the specific shape's reference to the View. Maybe you could provide more details about your scenario, so we can evaluate if these approaches are better than simply performing a dynamic_cast. – Emerson Cardoso Jan 4 '18 at 15:46
  • @ElevenJune - the whole point of having the view object is so your GUI doesn't need to know it is working with a class of type Square. The view object provides what's necessary to "view" the object (whatever you define that to be) and internally it knows that it is using an instance of a Square class. The GUI only interacts with the SquareView instance. Thus, you can't click on a 'Square' class. You can only click on a SquareView class. Changing parameters on the SquareView will update the underlying Square class.... – Dunk Feb 22 '18 at 23:32
  • ... This approach could very well let you get rid of your ShapeManager class. This will almost certainly simplify your design. I always say if you call a class a Manager then assume it's a bad design and figure something else out. Manager classes are bad for a myriad of reasons, most notably the god class problem and the fact that nobody knows what the class actually does, can do and can't do because Managers can do anything even tangentially related to whatever they are managing. You can bet those developers who follow you will take advantage of that leading to the typical big-ball-of-mud. – Dunk Feb 22 '18 at 23:33
  • 1
    ...you've already run into that problem. Why on earth would it make sense for a manager to be the one that changes a shape's dimensions? Why would a manager calculate a shape's perimeter? In case you didn't figure it out, I like the "Another suggestion". – Dunk Feb 22 '18 at 23:39

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.