Proper design to avoid the use of dynamic_cast?

Question

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*.

Answer 1

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.

Answer 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.