How can I remove dynamic_cast and if statements from this code snippet?

[Chris Stankevitz]

Well, conceptually, there's no difference between if/else and
dynamic_cast (and indeed OP's first post is dynamic_cast with if/
else). dynamic_cast merely moves decision to the type itself (whereas
if/else decides on the data). IOW, with dynamic_cast, type is used as
data.

Goran,

In the quote above are you trying to say this:

1. dynamic_cast and if/else are basically the same (for the reasons
you stated)
2. If someone is opposed to solving a problem with a bunch of if/else
statements based on data, then
3. that person should also be opposed to solving the problem using
dynamic_cast

Thank you,

Chris

 

[Guest]

Hello,

I would like to remove the "dynamic_cast" and "if" statements from the
code below. I believe some people would describe this as making the
code "polymorphic". Can you recommend a way to do this without
modifying the original classes in "Library A"?

My intention is to create an "XML Writer" class(es) for shapes without
putting "XML code" in the base classes. I plan to use a similar
pattern for drawing and other tasks my application has to perform on
Shape objects.

Thank you,

Chris

// Library A
struct Shape { virtual ~Shape() {} };
struct Circle : public Shape { float radius; };
struct Square : public Shape { float edge; };

// Library B
#include <iostream>

class XmlWriter
{
static void write(Shape* shape)
{
if (Circle* circle = dynamic_cast<Circle*>(shape))
{
std::cout << "<Circle Radius=" << circle->radius << "/>";
}
else if (Square* square = dynamic_cast<Square*>(shape))
{
std::cout << "<Square Edge=" << square->edge << "/>";
}
}
};

I think this is a typical usecase for the "visitor" pattern:

// Library A

class ShapeVisitor;
class Shape { public: virtual void accept(ShapeVisitor& visitor) = 0; };
class Circle : public Shape {
public:
virtual void accept(ShapeVisitor& visitor) { visitor.visit(*this); }
};
class Square : public Shape {
public:
virtual void accept(ShapeVisitor& visitor) { visitor.visit(*this); }
};
class ShapeVisitor {
virtual void visit(Square& square) = 0;
virtual void visit(Circle& circle) = 0;
};

// Library B
class XmlWriter : public ShapeVisitor {
virtual void visit(Square& square) {
// do something
}
virtual void visit(Circle& circle) {
// do something
}
void write(Shape& shape) {
shape.accept(*this);
}
};

Tobi

 

[Chris Stankevitz]

I think this is a typical usecase for the "visitor" pattern:

Tobi, this is exactly what I was looking for. Case closed.

Thank you,

Chris

 

[Werner]

Goran,

In the quote above are you trying to say this:

1. dynamic_cast and if/else are basically the same (for the reasons
you stated)
Wrong.


2. If someone is opposed to solving a problem with a bunch of if/else
statements based on data, then
3. that person should also be opposed to solving the problem using
dynamic_cast

Also wrong.

Firstly, as mentioned previously, one only needs dynamic_cast
if you have virtual inheritance. Static_cast will suffice most
of the time.

Secondly, there are many ways to kill a cat. Whether you use
names or casting, you're still comparing...

Thirdly, in your original example using dynamic_cast did
not violate OCP (and in my example even less).

Definition of OCP:

Open to extension, closed to modification:

Bad use of dynamic cast (open to modification):

if( dynamic_cast<Square*>(shape) )
{
}
else if( dynamic_cast<Circle*>(shape ) )
else if()
{
//etc...
}

Consider the acyclic visitor as example as described in
Agile Software Development (Robert C. Martin), where
dynamic_cast is put to good use.

Now, furthermore (Goran):

Show me how the use of dynamic cast in my example (or the
original example, for that matter), violates OCP. None
of the code using dynamic_cast (or static_cast as better
alternative) needs modification. OCP is not violated...

Do I need to modify existing classes to change logic?
No. Dynamic_cast usage does not imply OCP is violated,
only a certain style of dynamic_cast usage.

Kind regards,

Werner

 

[Werner]

Well, conceptually, there's no difference between if/else and
dynamic_cast (and indeed OP's first post is dynamic_cast with if/
else). dynamic_cast merely moves decision to the type itself (whereas
if/else decides on the data). IOW, with dynamic_cast, type is used as
data. And indeed, if new type introduced, conditional logic must be
changed with itand therefore open/closed principle is compromised.

No, conditional logic was not changed. The for loop ensured that...
But point to me how conditional logic changed by adding a new
class. Also (by referring to my example), show me how OCP is
violated by adding new drawers or shapes... For OCP to be
violated, code containing logic needs to change. The for
loop (whilst crude) suffices... When using a map, if/else
also exist (N times). You just don't need to modify it. The
same applies here. Therefore - nonsense.

Kind regards,

Werner

 

[Werner]

Tobi, this is exactly what I was looking for.  Case closed.

What happens when new shapes are added to the hierarchy?

Kind regards,

Werner

 

[Goran]

Goran,

In the quote above are you trying to say this:

1. dynamic_cast and if/else are basically the same (for the reasons
you stated)
2. If someone is opposed to solving a problem with a bunch of if/else
statements based on data, then
3. that person should also be opposed to solving the problem using
dynamic_cast

I guess you can put it that way.

Still, striving for polymorphism will (IMHO) lower the number "data-
based" if/else (or switch) decisions, even if some dynamic_cast stay.
Use of if/else where polymorphism would help is bad, dynamic_cast is
slightly better, appropriate abstraction (possibly well-known pattern,
if I can recognize it) is best ;-).

Goran.

 

[Goran]

I think this is a typical usecase for the "visitor" pattern:

// Library A

class ShapeVisitor;
class Shape { public: virtual void accept(ShapeVisitor& visitor) = 0; };
class Circle : public Shape {
public:
  virtual void accept(ShapeVisitor& visitor) { visitor.visit(*this); }};

class Square : public Shape {
public:
  virtual void accept(ShapeVisitor& visitor) { visitor.visit(*this); }};

class ShapeVisitor {
  virtual void visit(Square& square) = 0;
  virtual void visit(Circle& circle) = 0;

};

// Library B
class XmlWriter : public ShapeVisitor {
  virtual void visit(Square& square) {
    // do something
  }
  virtual void visit(Circle& circle) {
    // do something
  }
  void write(Shape& shape) {
    shape.accept(*this);
  }

};

Hmmm... I was thinking of "visitor", but found it inappropriate
because OP said he doesn't want to change library A, and visitor
requires changing the base class.

Goran.

 

[Goran]

Also wrong.

Firstly, as mentioned previously, one only needs dynamic_cast
if you have virtual inheritance. Static_cast will suffice most
of the time.

Not really. Indeed, OP needs dynamic_cast and has no virtual
inheritance.

Secondly, there are many ways to kill a cat. Whether you use
names or casting, you're still comparing...

Thirdly, in your original example using dynamic_cast did
not violate OCP (and in my example even less).

Definition of OCP:

Open to extension, closed to modification:

Bad use of dynamic cast (open to modification):

if( dynamic_cast<Square*>(shape) )
{}

else if( dynamic_cast<Circle*>(shape ) )
else if()
{
  //etc...

}

Consider the acyclic visitor as example as described in
Agile Software Development (Robert C. Martin), where
dynamic_cast is put to good use.

Now, furthermore (Goran):

Show me how the use of dynamic cast in my example (or the
original example, for that matter), violates OCP. None
of the code using dynamic_cast (or static_cast as better
alternative) needs modification. OCP is not violated...

Do I need to modify existing classes to change logic?
No. Dynamic_cast usage does not imply OCP is violated,
only a certain style of dynamic_cast usage.

I agree with you. TBH, I didn't look at your example much, nor was I
complaining about --your-- use of the dynamic_cast. I was complaining
about OP's use of it.

However, I think your use of dynamic_cast can be avoided, using the
same logic I applied in my first (I think) post. If your typed drawer
is --created-- with a reference to it's shape, then your DoDraw can be
using the correct type --without-- a cast at all (and Draw wouldn't
have a "shape" parameter at all). In my mind, you still made the same
mistake as the OP: you lost the original type of the Shape, and that
caused more code to be added.

BTW, your code breaks in unexpected ways if you derive from Square or
Circle: you might get SquareDrawer to draw, I dunno, DerivedSquare.

Goran.

 

[Goran]

I think this is a typical usecase for the "visitor" pattern:

// Library A

class ShapeVisitor;
class Shape { public: virtual void accept(ShapeVisitor& visitor) = 0; };
class Circle : public Shape {
public:
  virtual void accept(ShapeVisitor& visitor) { visitor.visit(*this); }};

class Square : public Shape {
public:
  virtual void accept(ShapeVisitor& visitor) { visitor.visit(*this); }};

class ShapeVisitor {
  virtual void visit(Square& square) = 0;
  virtual void visit(Circle& circle) = 0;

};

// Library B
class XmlWriter : public ShapeVisitor {
  virtual void visit(Square& square) {
    // do something
  }
  virtual void visit(Circle& circle) {
    // do something
  }
  void write(Shape& shape) {
    shape.accept(*this);
  }

};

Tobi

Visitor requires changing Shape and the rest of Lib A, which OP said
didn't want elsethread.

Goran.

 

[Bart v Ingen Schenau]

What happens when new shapes are added to the hierarchy?

Then the ShapeVisitor and derived visitors must be extended to support the
new shape.
This is no different from the original code with an if-else ladder based on
the result from dynamic_cast, except that the compiler is better able to
help warn you if you forgot to update a visitor.

Kind regards,

Werner

Bart v Ingen Schenau

 

[Tobias Müller]

Visitor requires changing Shape and the rest of Lib A, which OP said
didn't want elsethread.

Goran.

Fair point. However, when reading the rest of the thread I had the
impression that what the OP really wanted is not to leave Lib A unchanged,
but rather not to include any XML specific features.
And a vistior interface is quite abstract and useful for a library like A.

And apparently the OP is happy with that solution.

Tobi.

 

[Noah Roberts]

Hello,

I would like to remove the "dynamic_cast" and "if" statements from the
code below.  I believe some people would describe this as making the
code "polymorphic".  Can you recommend a way to do this without
modifying the original classes in "Library A"?

My intention is to create an "XML Writer" class(es) for shapes without
putting "XML code" in the base classes.  I plan to use a similar
pattern for drawing and other tasks my application has to perform on
Shape objects.

The adapted visitor pattern that people have shown here is one good
way to solve this. Another you could consider though is a multi-
dispatch mechanism separate from the two parts. This would retain the
dynamic_cast stuff, but you can use TMP to build that. Review _Modern
C++ Design_ by Alexandrescu.

Basically, something like so:

template < typename Seq, typename Enable = void >
struct dispatcher
{
typedef typename front<Seq>::type my_type;
typedef typename pop_front<Seq>::type next_seq;

static void write(Shape const& sh, Writer & wr)
{
if (my_type const* csh = dynamic_cast<my_type const*>(&sh))
wr.write(*csh);
else
dispatcher<next_seq>::write(sh);
}
}

template < typename Seq >
struct dispatcher<Seq, typename enable_if<empty<Seq> >::type>
{
static void write(Shape const&, Writer &) { throw
std::runtime_error("Unknown shape type."); }
};

void write_shape(Shape const& sh, Writer & w)
{
typedef vector<Square, Circle> shape_seq;
dispatcher<shape_seq>::write(sh,w);
}

 

[Chris Stankevitz]

Visitor requires changing Shape and the rest of Lib A, which OP said
didn't want elsethread.

However, when reading the rest of the thread I had the
impression that what the OP really wanted is not to leave Lib A unchanged,
but rather not to include any XML specific features.

You are both correct. At first I said "without modifying LibA" to
discourage "just put the XML specific feature in LibA" (turned out
this didn't discourage the first few replies). Later, as it became
clear I did not pose my question well, my question evolved. As Tobi
picked up, I did not want XML specific features in Library A but was
willing to make changes to Library A. However, I can imagine
scenarios in which LibA is not alterable -- in such a case Tobi's
solution would not work. I apologize for leading the group to believe
that.

And a vistior interface is quite abstract and useful for a library like A..

And apparently the OP is happy with that solution.

Yes! Not only was I happy with it, when I saw it I knew immediately
that this was what I was thinking of -- I just didn't have a name for
it or know how to go about it or even know if it was possible with c+
+.

Thank you,

Chris