C++ How to I replace this if...else statement?

Question

I have the following C++ code (simplified version):

class Shape
{
    bool isCircle = false;
    bool isSquare = false;
}

class Circle : public Shape
{
    // some special members/methods
}

class Square : public Shape
{
    // some special members/methods
}

class CAD
{
    virtual DrawCircle(Circle * circle) = 0;
}

class SWX : public CAD
{
    virtual DrawCircle(Circle * circle){// do some stuff that draws circle on SWX system}
}

class PRO : public CAD
{
    virtual DrawCircle(Circle * circle){// do some stuff that draws circle on PRO system}
}

int main()
{
    Circle * circle = new Circle();
    circle->isCircle = true;

    Square * sq = new Square;
    sq->isSquare = true;

    vector<Shape*> shapes;
    shapes.push_back(circle);
    shapes.push_back(sq);

    SWX * swx = new SWX();

    for( int i = 0 ; i < shapes.size() ; ++i )
    {
        if( shapes[i]->isCircle )
    {
        SWX->DrawCircle((Circle*)(shapes[i]));
    }
    else if( shapes[i]->isSquare )
    {
        SWX->DrawSquare((Square*)(shapes[i]));
    }
}

I wish to remove the need for if...else (if at all possible within the constraints stated below).

My constraints right now are:

• The CAD and derived classes are ginormous classes with various external dependencies.
• The CAD classes cannot be merged with the Shape and derived classes (that would have been ideal, since then I can use polymorphism to solve my problem), since other projects/classes depend on the Shape classes and cannot depend on the CAD classes.
• There are more than a dozen Shape-derived classes with a half dozen CAD-derived classes and this if...else is happening in numerous locations - so it would help if any solution is simple to understand (easier to convince my teammates to change legacy code).

Any suggestions/comments/solution you have would be most welcome.

Answer 1

The standard solution for this problem, especially given your constraints regarding dependencies, is to use the Visitor Pattern.

Here's how Visitor Pattern would work in your case:

• You need an abstract ShapeVisitor class. It has an abstract Visit method for each concrete subclass of Shape. eg: Visit(Circle*), Visit(Square*), etc.
• Shape has an abstract AcceptVisitor(ShapeVisitor*) method.
• Each Shape subclass implements AcceptVisitor as just calling visitor->Visit(this)
• Each CAD class is a (or has-a, up to you) a ShapeVisitor. The Visit methods do the appropriate drawing for the specific type of Shape. No conditional or casting required.

Here's a modified version of your code that uses Visitor Pattern in a pretty low-impact way:

class Circle;
class Square;
class ShapeVisitor
{
    virtual void Visit(Circle *circle) = 0;
    virtual void Visit(Square *square) = 0;
}

class Shape
{
    virtual void AcceptVisitor(ShapeVisitor *visitor) = 0;
}


class Circle : public Shape
{
    // some special members/methods

    virtual void AcceptVisitor(ShapeVisitor *visitor)
    {
        visitor->Visit(this);
    }
}

class Square : public Shape
{
    // some special members/methods

    virtual void AcceptVisitor(ShapeVisitor *visitor)
    {
        visitor->Visit(this);
    }
}

class CAD : public ShapeVisitor
{
    virtual DrawCircle(Circle *circle) = 0;
    virtual DrawSquare(Square *square) = 0;

    virtual void Visit(Circle *circle) {
        DrawCircle(circle);
    }

    virtual void Visit(Square *square) {
        DrawSquare(square);
    }
}

class SWX : public CAD
{
    virtual DrawCircle(Circle *circle){// do some stuff that draws circle on SWX system}

}

class PRO : public CAD
{
    virtual DrawCircle(Circle * circle){// do some stuff that draws circle on PRO system}
}

int main()
{
    Circle * circle = new Circle();
    Square * sq = new Square;

    vector<Shape*> shapes;
    shapes.push_back(circle);
    shapes.push_back(sq);

    SWX * swx = new SWX();

    for( int i = 0 ; i < shapes.size() ; ++i )
    {
        shapes[i]->AcceptVisitor(SWX);
    }
}

In this code I've opted for making CAD actually a subclass of ShapeVisitor. Also, since you've already got virtual methods in CAD to do the drawing, I implemented the Visit methods there (once), rather than once in each subclass. Once you switch clients over to the using AcceptVisitor instead of calling the Draw* methods directly you could make those methods protected, and then eventually move the implementation of the Visit methods down to the subclasses (that is: refactor to remove the extra level of indirection caused by having Visit(Foo*) call DrawFoo(Foo*)).

Answer 2

You've got some rather wonky OO going on there, but at the very least DrawXxxx should just become Draw(). Circle, Square, and the other shapes would then define a Draw() method that provides an implementation for a virtual Draw method on Shape. Then you can just call Draw on any Shape and it will do the right thing.

The isXxxx booleans should go too. Classes know what they are and instanceof can tell you (though it shouldn't be necessary to check when you Draw since that will be a virtual method invocation).

Answer 3

This is a classic case for DoubleDispatch, where you need to have a separate method for each possible (Shape, CAD) pair:

• Nuke the isSquare/isCircle fields.
• add virtual void DrawOn(CAD*) to the Shape interface.

• Implement Circle::DrawOn(CAD*) (for example):

  void Circle::DrawOn(CAD *c) {
    c->DrawCircle(this);
  }
  

  This is the "trick" which allows a call like myCircle->DrawOn(mySWX) to call the correct method no matter the type of the Shape or the CAD.

Answer 4

This may not be the ideal solution, but you could just provide an CAD::Draw member function and overload it to handle each different type of shape. Something like:

class CAD {
  public:
    virtual void Draw(const Circle& circle) = 0;
    virtual void Draw(const Square& square) = 0;
};

Then you could just call Draw on any supported object without any if statements.

Answer 5

Why not just SWX->Draw(shapes[i]);? You would need to add two Draw method with one that takes a Circle and one that takes a square?

Answer 6

This is pretty basic OO polymorphism. Circle and Square are specialized versions of shape. They should each know what specialized behavior is needed to deal with your cad classes.

class Shape
{
    virtual void DrawWithCAD(CAD * cad) = 0;
}

class Circle : public Shape
{
    virtual void DrawWithCAD(CAD * cad)
    {
        cad->DrawCircle(this);
    }
}

class Square : public Shape
{
    virtual void DrawWithCAD(CAD * cad)
    {
        cad->DrawSquare(this);
    }
}

Then your main() loop would change to:

for( int i = 0 ; i < shapes.size() ; ++i )
{
    shapes[i]->DrawWithCAD(swx);
}

Answer 7

The way I would solve this is to let the individual shape classes draw themselves onto some kind of drawing "canvas" (a class providing a drawing buffer and some fundamental drawing functions, e.g. for lines, rectangles, and ellipses).

The SWX class would then call the Shape's Draw method, providing it with its own canvas:

class Shape
{
public:
    virtual void DrawOnto(Canvas& canvas) = 0;
};

class Circle : public Shape
{
public:
    virtual void DrawOnto(Canvas& canvas);
};

void Circle::DrawOnto(Canvas& canvas)
{
    // ..., e.g.:
    canvas.DrawEllipse(center.x, center.y, radius, radius);
}

...

class SWX : public CAD
{
private:
    Canvas canvas;
public:
    void Draw(Shape& shape)
    {
        shape.DrawOnto(this.canvas);
        // ^ this is the place where your if..else used to be
    }
};

Answer 8

Write a several global Draw functions overloaded for each Shape... (DrawCircle and friends are virtual so we can use polymorphism for calls to CAD objects)

void Draw(CAD *cad, Circle *circle){
    cad->DrawCircle(circle);
}

void Draw(CAD *cad, Square *square){
    cad->DrawSquare(square);
}

Answer 9

Why not define a simple ICAD interface? Since CAD depends on Shape it does not increase complexity:

class Shape
{
    Draw(ICAD* cad) = 0;        
}

class Circle : public Shape
{
    Draw(ICAD* cad)
    {
        ICAD->DrawCircle(self)
     }
}

class Square : public Shape
{
    Draw(ICAD* cad)
    {
        ICAD->DrawSquare(self)
     }
}

DrawSquare(self) looks funny, but I don't know what the CAD classes do with the shape objects.

class ICAD
{
    virtual DrawSquare(Square* square) = 0;
    virtual DrawCircle(Circle * circle) = 0;
}

I assume class CAD has more than those abstract methods, which is why you don't couple it with Shape.

class CAD : public ICAD
{
    // big CAD class...
}

class SWX : public CAD
{
    virtual DrawCircle(Circle * circle){// do some stuff that draws circle on SWX system}
}

class PRO : public CAD
{
    virtual DrawCircle(Circle * circle){// do some stuff that draws circle on PRO system}
}

int main()
{
    Circle * circle = new Circle();
    Square * sq = new Square;

    vector<Shape*> shapes;
    shapes.push_back(circle);
    shapes.push_back(sq);

    SWX * swx = new SWX();

    for( int i = 0 ; i < shapes.size() ; ++i )
    {
        shapes[i]->Draw(swx);
    }
}