Simple OOP, polymorphism design question

[Jacek Dziedzic]

Hello!

First of all please forgive me for not posting a compilable snippet,
but rather a simplified piece of code with the unimportant
details left out.

Let's say I have two classes 'box_shape' and 'cylinder_shape ' that
derive from a common base class 'shape', more or less like this:

namespace Shapes {

int SHA_UNDEFINED=-1;
int SHA_BOX=0;
int SHA_CYLINDER=1;

class shape {
public:
shape();
virtual ~shape();

int virtual what_shape() const {
return SHA_UNDEFINED;
}

void virtual move_shape(/*...*/) {
/*...*/
}

/* some more methods common to all shapes*/
};

class box_shape : public shape {
public:
box_shape();

int what_shape() const {
return SHA_BOX;
}

void move_shape(/*...*/) {
shape::move_sha pe(/*...*/);
/* do some stuff specific to box_shape */
}

vector<double> box_corners() const { // returns box corners
/*...*/
}
};

class cylinder_shape : public shape {
public:
cylinder_shape( );

int what_shape() const {
return SHA_CYLINDER;
}

void move_shape(/*...*/) {
shape::move_sha pe(/*...*/);
/* do some stuff specific to cylinder_shape */
}

double cylinder_height () const { // returns cylinder height
/*...*/
}
};

} // of namespace Shapes

Later on I have a class called 'system_geometr y' which
contains a member 'region', which is a shape, either a box-like or
a cylindrical shape. Since I don't know in advance which
particular descendant of 'shape' this region is, I decided
to use a pointer to shape, like this:

class system_geometry {
public:
/* ... lots of stuff here ... */
Shapes::shape *region;
}

and I initialize it like that:

if(user_wants_a _box) region=new Shapes::box_sha pe;
else if(user_wants_a _cylinder) region=new Shapes::cylinde r_shape;

Thanks to polymorphism I can now have calls like
region->what_shape() or region->move_shape() which
dynamically resolve to methods of appropriate descendant classes.

That's all right up to this point. There are some places in
my code, however, when I need to call some methods that are
specific to the derived classes, something akin to:

switch(region.w hat_shape()) {
case SHA_BOX:
// this branch now needs to call region->box_corners( );
break;
case SHA_CYLINDER:
// this branch now needs to call region->cylinder_heigh t();
break;
default:
throw EInternalError( "Wrong shape");
}

The problem is, of course, that neither box_corners() nor
cylinder_height () are defined for the base class 'shape'.
What should I do in this situation? I've thought up two
solutions. One, possibly ugly, is to add all the names of
all methods of the derived classes to the base class as
pure virtual methods. That way 'shape' will formally have
all the required methods. I don't like that solution.

The other solution I've been pondering is via dynamic_cast.
Will it be all right to do something like:

switch(region.w hat_shape()) {
case SHA_BOX:
box_shape *region_as_box= dynamic_cast<bo x_shape*>(regio n);
// now this branch can use region_as_box->box_corners( )
break;
case SHA_CYLINDER:
cylinder_shape *region_as_cyli nder=
dynamic_cast<cy linder_shape*>( region);
// now this branch can use region_as_cylin der->cylinder_heigh t();
break;
default:
throw EInternalError( "Wrong shape");
}

I have doubts because I recall that casting base --> derived
is not a good idea. On the other hand I can be sure that 'region'
definitely points to an instance of a derived class, not the base.
And I guess I'm trying to cast base* --> derived*, but I'm a little
confused here.

Is this approach OK? If not, how can I solve this problem?

TIA,
- J.

[Derrick Coetzee]

Re: Simple OOP, polymorphism design question

Jacek Dziedzic wrote:[color=blue]
> switch(region.w hat_shape()) {
> case SHA_BOX: [ . . . ]
> case SHA_CYLINDER: [ . . . ]
> }[/color]

Whenever you see a switch on the type of an object, this is a good sign
that you are failing to exploit polymorphism properly. For example,
consider this code:

class Animal { ... };
class Dog : Animal { ... };
class Cat : Animal { ... };
....
void makeSound(Anima l* animal) {
switch(animal->getType()) {
case TYPE_DOG:
Dog* dog = static_cast<Dog *>(animal);
cout << "Woof!";
dog->wagTail();
break;
case TYPE_CAT:
Cat* cat = static_cast<Cat *>(animal);
cout << "Meow!";
cat->purr();
break;
}
}

As you've surely guessed, this is the wrong way to implement
makeSound(), and the above casts, while safe and acceptable, are a
symptom of bad design. There should instead be a virtual member function
on Animal which is overridden accordingly in Dog and Cat:

class Animal { ... virtual void makeSound() = 0; };
class Dog : Animal { ... virtual void makeSound(); };
class Cat : Animal { ... virtual void makeSound(); };
....
void Dog::makeSound( ) {
cout << "Woof!";
wagTail();
}
void Cat::makeSound( ) {
cout << "Meow!";
cat->purr();
}

The main advantage of this design is modularity: if you add a new
animal, you don't have to go hunting down switches and adding branches
to them in various places. Instead, you just add a new class defining
each of the virtual methods. This also ensures no cases are missed, and
avoids ugly code.

However, if this was, for some reason, impossible (maybe Animal is from
a library header that can't be changed) then the switch-based solution
would work and may be the best possible solution, at least in the
short-term. If you're still worried about the generally unsafe
static_casts, you may want to use dynamic_casts instead. In particular,
when you use dynamic_casts with references they conveniently throw an
exception when the run-time type is wrong.

I hope this helps, although I'm not sure how much of it applies to your
specific case.
--
Derrick Coetzee
I grant this newsgroup posting into the public domain. I disclaim all
express or implied warranty and all liability. I am not a professional.

[David Hilsee]

Re: Simple OOP, polymorphism design question

"Jacek Dziedzic" <jacek__NOSPAM_ _@janowo.net> wrote in message
news:chaps9$drc $1@korweta.task .gda.pl...[color=blue]
> Hello!
>
> First of all please forgive me for not posting a compilable snippet,
> but rather a simplified piece of code with the unimportant
> details left out.
>
> Let's say I have two classes 'box_shape' and 'cylinder_shape ' that
> derive from a common base class 'shape', more or less like this:
>
> namespace Shapes {
>
> int SHA_UNDEFINED=-1;
> int SHA_BOX=0;
> int SHA_CYLINDER=1;
>
> class shape {
> public:
> shape();
> virtual ~shape();
>
> int virtual what_shape() const {
> return SHA_UNDEFINED;
> }
>
> void virtual move_shape(/*...*/) {
> /*...*/
> }
>
> /* some more methods common to all shapes*/
> };[/color]

If you use constants and a what_shape() function, then you are essentially
using RTTI. From a design standpoint, it's about the same as using
dynamic_cast to figure out the type of the object because you still end up
using switch/case-like code.

<snip>[color=blue]
> That's all right up to this point. There are some places in
> my code, however, when I need to call some methods that are
> specific to the derived classes, something akin to:
>
> switch(region.w hat_shape()) {
> case SHA_BOX:
> // this branch now needs to call region->box_corners( );
> break;
> case SHA_CYLINDER:
> // this branch now needs to call region->cylinder_heigh t();
> break;
> default:
> throw EInternalError( "Wrong shape");
> }
>
> The problem is, of course, that neither box_corners() nor
> cylinder_height () are defined for the base class 'shape'.
> What should I do in this situation? I've thought up two
> solutions. One, possibly ugly, is to add all the names of
> all methods of the derived classes to the base class as
> pure virtual methods. That way 'shape' will formally have
> all the required methods. I don't like that solution.
>
> The other solution I've been pondering is via dynamic_cast.
> Will it be all right to do something like:
>
> switch(region.w hat_shape()) {
> case SHA_BOX:
> box_shape *region_as_box= dynamic_cast<bo x_shape*>(regio n);
> // now this branch can use region_as_box->box_corners( )
> break;
> case SHA_CYLINDER:
> cylinder_shape *region_as_cyli nder=
> dynamic_cast<cy linder_shape*>( region);
> // now this branch can use region_as_cylin der->cylinder_heigh t();
> break;
> default:
> throw EInternalError( "Wrong shape");
> }
>
> I have doubts because I recall that casting base --> derived
> is not a good idea. On the other hand I can be sure that 'region'
> definitely points to an instance of a derived class, not the base.
> And I guess I'm trying to cast base* --> derived*, but I'm a little
> confused here.[/color]

Many people don't like constructs like this because they can cause
maintenance problems. Adding a new shape becomes more complicated because
it requires you to dig through the codebase, updating various switch/case or
if/else type-specific operations. If you have many of these, it can be a
real nightmare.

Usually, if you want to avoid these switch/case tests, you add an additional
virtual function that contains the type-specific behavior. For example, if
it's drawing on the screen, then you could add an additional virtual member
function to Shape called, say, draw(). Could you do that for this
switch/case using function, or is that approach unreasonable?

--
David Hilsee

[Derrick Coetzee]

Re: Simple OOP, polymorphism design question

Derrick Coetzee wrote:[color=blue]
> void Cat::makeSound( ) {
> cout << "Meow!";
> cat->purr();
> }[/color]

Err, I meant:

void Cat::makeSound( ) {
cout << "Meow!";
purr();
}

--
Derrick Coetzee
I grant this newsgroup posting into the public domain. I disclaim all
express or implied warranty and all liability. I am not a professional.

[Phlip]

Re: Simple OOP, polymorphism design question

Jacek Dziedzic wrote:
[color=blue]
> First of all please forgive me for not posting a compilable snippet,[/color]

The "compilable snippet" rule is for syntax questions.
[color=blue]
> Let's say I have two classes 'box_shape' and 'cylinder_shape ' that
> derive from a common base class 'shape', more or less like this:[/color]
[color=blue]
> class shape {[/color]
[color=blue]
> int virtual what_shape() const {[/color]
[color=blue]
> };[/color]
[color=blue]
> class box_shape : public shape {[/color]
[color=blue]
> int what_shape() const {[/color]
[color=blue]
> vector<double> box_corners() const { // returns box corners[/color]
[color=blue]
> };[/color]
[color=blue]
> class cylinder_shape : public shape {[/color]
[color=blue]
> int what_shape() const {[/color]
[color=blue]
> double cylinder_height () const { // returns cylinder height[/color]
[color=blue]
> };[/color]
[color=blue]
> Later on I have a class called 'system_geometr y' which
> contains a member 'region', which is a shape, either a box-like or
> a cylindrical shape. Since I don't know in advance which
> particular descendant of 'shape' this region is, I decided
> to use a pointer to shape, like this:
>
> class system_geometry {
> public:
> /* ... lots of stuff here ... */[/color]

Uh, why "lots" of stuff? Could it move elsewhere?
[color=blue]
> Shapes::shape *region;
> }
>
> and I initialize it like that:
>
> if(user_wants_a _box) region=new Shapes::box_sha pe;
> else if(user_wants_a _cylinder) region=new Shapes::cylinde r_shape;[/color]

This is correct, so far. The situation "user_wants_a_f oo" is a _boundary_
situation. It might represent types as typecodes. The first layer, going in,
should convert type codes to types. Code distant from such a boundary should
use as few 'if' and 'switch' statements as possible.
[color=blue]
> Thanks to polymorphism I can now have calls like
> region->what_shape() or region->move_shape() which
> dynamically resolve to methods of appropriate descendant classes.[/color]

And you obey the Liskov Substitution Principle, such that clients never
accidentally become aware what target type they use, right?
[color=blue]
> That's all right up to this point. There are some places in
> my code, however, when I need to call some methods that are
> specific to the derived classes, something akin to:
>
> switch(region.w hat_shape()) {
> case SHA_BOX:
> // this branch now needs to call region->box_corners( );
> break;
> case SHA_CYLINDER:
> // this branch now needs to call region->cylinder_heigh t();
> break;
> default:
> throw EInternalError( "Wrong shape");
> }
>
> The problem is, of course, that neither box_corners() nor
> cylinder_height () are defined for the base class 'shape'.
> What should I do in this situation?[/color]

You seek to avoid a down-cast. However, your style got poor when you called
region->what_shape() itself. Clients should tell, not ask, servant objects
what to do.

What is the switch, semantically, for? Suppose that box_corners() return a
vector of 4 points, and cylinder_height () returned 2. Then something used
them to decorate 4 or 2 times.

Whatever it does, suppose system_geometry inherited an abstract base class,
System, with a pure virtual method called 'emblandish()'. Then you call:

region->emblandishYour self(*this);

The base class provides a pure virtual emblandishYours elf(System & aSystem)
method. Then the box provides a concrete implementation, and calls
aSystem.embland ish(point) four times, and cylinder calls it two times.

This idea is not "better". The point is to decouple interfaces at the same
time as we merge duplicated definitions of behavior. Read the book /Design
Patterns/.
[color=blue]
> I've thought up two
> solutions. One, possibly ugly, is to add all the names of
> all methods of the derived classes to the base class as
> pure virtual methods. That way 'shape' will formally have
> all the required methods. I don't like that solution.[/color]

Sometimes that's just "right". You won't find it in /Design Patterns/,
though... ;-)
[color=blue]
> The other solution I've been pondering is via dynamic_cast.
> Will it be all right to do something like:
>
> switch(region.w hat_shape()) {
> case SHA_BOX:
> box_shape *region_as_box= dynamic_cast<bo x_shape*>(regio n);
> // now this branch can use region_as_box->box_corners( )
> break;
> case SHA_CYLINDER:
> cylinder_shape *region_as_cyli nder=
> dynamic_cast<cy linder_shape*>( region);
> // now this branch can use region_as_cylin der->cylinder_heigh t();
> break;
> default:
> throw EInternalError( "Wrong shape");
> }[/color]

Sometimes that is "right" too. The proof comes when this system must change,
somehow. How easily could you add another shape? How easily could you change
cylinder_height () without changing box_corners()?

(And, sometimes, deferring that proof until you find a real need to change
will teach what kind of decoupling you actually need... Read the book
/Refactoring/ to learn how to re-design existing code. Then read
/Refactoring to Patterns/ to put them together!)

--
Phlip

http://industrialxp.org/community/bin/view/Main/TestFirstUserInterfaces

[Jacek Dziedzic]

Re: Simple OOP, polymorphism design question -&gt; EOT


Thanks a lot for all the answers. Yes, it's late at night here
in Europe, so I missed the (now) obvious solution which was to
move the code to the descendants of 'shape' and make them
perform tasks themselves instead of asking what type they were
and performing tasks for them.

The method in question was 'make_periodic_ with_respect_to _shape(...)'
which now just looks like
{
region->make_periodic_ with_respect_to _shape(...);
}

letting the shape-classes do their tasks by themselves.

Owing to your hints I've managed to remove all but one of my
'switch(type_of )' constructs and the code looks much cleaner now!
Adding more shapes will definitely be easier now, as those
switch statements reminded me of Fortran-77 :).

Thanks a lot!

- J.

[Phlip]

Re: Simple OOP, polymorphism design question -&gt; EOT

Jacek Dziedzic wrote:
[color=blue]
> The method in question was 'make_periodic_ with_respect_to _shape(...)'
> which now just looks like
> {
> region->make_periodic_ with_respect_to _shape(...);
> }
>
> letting the shape-classes do their tasks by themselves.[/color]

Forgot to mention one more goal: Dependency Inversion Principle. Look that
up, ensure you have it, and _then_ you are done.

--
Phlip

http://industrialxp.org/community/bin/view/Main/TestFirstUserInterfaces