Fog Creek Software
Discussion Board




C++ OO question

I've been staring this problem in the face for a few hours, and my brain is officially goo.  Tomorrow I'll spend perusing the GoF's Design Pattern book - unless someone here can help.

This is a contrived example, so take it easy on the logical goofiness.  Let's say you have some shape classes - Triangle, Square, and Circle - and some drawing classes - Pen, Pencil, and Brush.

Now, the shape classes all inherit from IShape, and the drawing classes all inherit from IDrawing.  So this let's me do things like this:

void IDrawing::ColorBackground(IShape* pShape)
{
    pShape->Color(m_colorBackground);
}

And all is well (assuming you don't object to abstract base classes having implementation, but that's a different argument).  But, what happens when I want to give the shape new coordinates?  I'd like to have a pure virtual method like this:

    virtual void IShape::SetCoords( mumble ) = 0;

And then override it in each shape class.  But the problem is, of course, each IShape-derived class needs a different signature:

    virtual void Triangle::SetCoords(int n1, int n2, int n3);
    virtual void Square::SetCoords(Rect rc);
    virtual void Circle::SetCoords(Point center, int nRadius);

This means I can't call SetCoords( ) polymorpically via IShape, which annoys me.  I have to have a method in each class, and then I add the Rhombus class, I have to remember to implement SetCoords( ).  Seems untidy and un-OO.

What am I missing here?  Thanks.

Grumpy Old-Timer
Thursday, September 18, 2003

Maybe you can add a new class ICoord so that the SetCoord() functions become:

void IShape::SetCoords(const ICoord &coord);

So each shape can ask ICoord for its coordinates in a suitable form.

Mark Watson
Thursday, September 18, 2003

"I have to have a method in each class, and then I add the Rhombus class, I have to remember to implement SetCoords( ).  "

How else would you do it?  OO won't magically write code for you.  : )  The methods are different, so they must be implemented separately.  I am talking about this example specifically, so maybe your real example is different.  Whether you can generalize the method depends totally on the problem domain.

You can force them to have the same interface as the poster above suggested, but then you have the ICoord hierarchy which presumably would match your IShape hierachy.  That is, if you add a Rhombus class inherting from IShape, then you would probably have to add an RhombusCoord class derived from ICoord.  Although of course it depends on what exactly you are doing (maybe you could share rhombus and square coordinates or something).

This may or may not be a bad idea depending if there is a lot of Coord specific functionality in your classes and a lot of non-Coord specific functionality.  But in general it is a disadvantage to have parallel hierarchies.  I think this issue is discussed in the design patterns book.

Some advice I recently got, and which I've found to be true in practice, is to try _generalize_ from concrete classes, rather than create abstract classes and try to _specialize_.  There are always hairs and craggles in the real world that you won't see if you think too abstractly.
So write all your Triangle, Square, and Circle classes, and then afterward see what you can factor out into IShape.

Andy
Thursday, September 18, 2003

Why not make the shape implement transforms like translation and scale instead of mucking directly with its state? *duck*

nonanon
Thursday, September 18, 2003

That's a good point, assuming that the OP is implementing a drawing program, which it certainly sounds like.  As always the design depends on the specifics.  But you're right that you should try to use messages sent to shape objects (scale along this axis, translate in this direction, rotate x radians) and those lend themselves very well to polymorphic functions.  The SetCoords actually reminds me of the getters/setters discussion recently... should be avoided if possible, assuming the coordinates are internal state.  Clients should avoid manipulating the internal state directly, and just request logically what they need to be done.  Missed that in my first post...

Andy
Thursday, September 18, 2003

" But in general it is a disadvantage to have parallel hierarchies.  I think this issue is discussed in the design patterns book."

I andy...Im interested in the disadvantages, care to give me a quick run down?  <g> or maybe a online link if your memory is less than perfect..

FullNameRequired
Thursday, September 18, 2003

I think part of the problem here is that Triangle, Square, and Circle are not good subclasses of Shape.  They are not substitutable for Shape, generally.

It's that whole "why a circle is not an ellipse" discussion, which Marshall Kline covers in excellent detail in the C++ FAQ Lite.

That is to say, radius is a reasonable property for a circle to have, but not a square.  So everywhere you see a shape, it cannot be either a square or a circle, because it doesn't make sense for both to have a radius property.  The end result is a lot of code that tests for "unsupported" properties, or worse, run-time type checking.

If you are set on going down this route, I think the best way to go is to pass those properties to the objects' constructors, and then let them handle them internally; ie, they should be totally encapsulated.

Short of that, it's really not a matter of design patterns, but factoring and object discovery.  As has been said, it's hard to give concrete advice without a more concrete scenario.  And I'm far too lazy to invent one at this late hour.  :)

As for the ICoord idea, I don't see how that solves any problems.  It just delegates the non-substitutability to another class hierarchy.

c++_conventioner
Friday, September 19, 2003

For more info about "a circle is not a kind-of an ellipse":

http://www.parashift.com/c++-faq-lite/proper-inheritance.html#faq-21.6

You should NEVER have a method in a base class which doesn't make sense in all of the derived classes.  If you do, then you'll be unable to treat them generically.  If you have Circle.setRadius(int) and Ellipse.setRadii(int,int), then it will be unnatural to make one derive from the other.

Intuitively it might make sense that circles are ellipses, but in this case your intuition is leading you down a primrose path: you can do some things with ellipses that you can't with circles.

Alyosha`
Friday, September 19, 2003

Just a thought.

Going by the original problem, here what I thought.
IShape<-iLine, IShape<-IPolygon<-Triangle, IShape<-IPolygon<-Square, IShape<-IPolygon<-Circle, IShape<-IPolygon<-Ellipse.
IPolygon will contains ILine objects as polygons are made of lines. Triangle, Square are polygons with particular no of sides. Circle, Ellipse is a polygon with infinite sides. (Well correct me my geometry is not that good).

Now as of overriding is concerned. I would like to suggest
IPolygon::SetSides(const ICoord&coord);
would work fine as Mark suggested.

I just had this passing idea!!!!

cooler
Friday, September 19, 2003

I agree with everyone about the the eclipse != circle etc stuff.

But for quick progress, describe shapes by their extents?

i like i
Friday, September 19, 2003

This property of inheritance is described by the Liskov Substitution Principle.  (see: http://www.objectmentor.com/resources/articles/lsp.pdf)  The basis for this principle lies in the theory behind Design by Contract in that it is always necessary for a subclass to honor at least the contract guaranteed by its superclass.  If it is impossible to meet this requirement, then the subclass should not have the superclass that it does.

This is why there is a problem with circles and ellipses.  Ellipses assert that scaling in the x and y directions are independent.  Circles cannot honor this contract and still remain a Circle.

The ellipse/circle thing can be resolved if you make Shapes immutable and make transformations on Shapes return new Shapes.  In this design, a circle that is scaled differently in two directions would morph into a proper ellipse as geometry would lead you to expect.

Also, the discussion above about Shape being a poor superclass for Squares and Circles because Squares can't have radii is flawed.  It would only hold if the radius was defined on Shape, which it is not. 

anon
Friday, September 19, 2003

FullName: I don't remember the discussion off hand, but one practical problem is that it is a form of dependency you add in your code.  Invariably someone else will work on your code, and hack up a solution where they derived from one base class but neglected to derive from the other.  Maybe there wasn't a full discussion in the book... I seem to remember under the Advantages/Disadvantages of a certain pattern, it was a disadvantage that you had to maintain parallel class hierarchies.  As always it's a tradeoff.  It could provide enough benefit to justify this maintenance problem (or I'm sure they wouldn't have included it in the book : ) ).

cooler: Without being a jerk, it seems amazing that you got that from the original problem... considering there wasn't one, it was 6 hypothetical classes.  Whether you need a Line class or not depends totally on the problem you're solving.  And in any example I can think of, you would not want to derive shape from line.  I think you mean Shapes would _contain_ lines, not _be_ them (what they call "is a" vs. "has a").  The fact that Circle and Ellipse could be mathematically described as polygons in the limit doesn't seem to have any relevance here.  If Polygon stores a list of line segments, would you have an infinite list?  It seems like you are falling into the trap of trying to capture the "real world" as described in the C++ FAQ link.

Also I'd point out that Triangle/Square/Circle could be perfectly good subclasses of Shape in one program, but terrible ones in another program.  It doesn't make much sense to talk in the abstract.  It depends on exactly what data and methods each class has.

Andy
Friday, September 19, 2003

"I seem to remember under the Advantages/Disadvantages of a certain pattern, it was a disadvantage that you had to maintain parallel class hierarchies."

I can see it I guess, maintaining parallel class hierarchies to play with each other basically means that the classes are specifically designed to play with each other, which implies a definite dependency on the internal implementation even it its not obvious  in the overview.

Interesting, Ive done exactly this kind of thing before and it never felt quite right but I just assumed thats how it had to work...I must read up on the alternatives..

FullNameRequired
Friday, September 19, 2003

"It would only hold if the radius was defined on Shape, which it is not. "

I think you're missing the point of the OP, which was that he wanted to define "SetCoords" _polymorphically_.  Clearly the class hierarchy is not well-suited to this.  As it is, you're right, it's fine, but it doesn't currently meet OP's requirements for reusability.

smkr45
Friday, September 19, 2003

if your dreaming of a function SetCoords() that in the base class takes BaseParamType, and in the derived class takes DerivedParamType, you are dreaming of argument covarience.  Unfortunately, C++ doesn't allow it, because it goes against the grain of the type checker.  I guess you could do it anyway with a dynamic_cast.

Keith Wright
Friday, September 19, 2003

As others have said, your best bet is really translating and scaling (which is a common function for all shapes), instead of trying to set some numbers.

That's where the second problem comes in: You are not trying to set "Coordinates". You are trying to set properties specific to the particular shape.

In general, a lot of those mind-numbing questions are the result of being stumped by the names chosen. Had you instead created functions like setSideLength(), setRadius(), it would have been obvious that they don't do the same thing, and that polymorphism won't work for you, here.

What it boils down to (for me, at least) is: If you're programming, spend some time on choosing proper names. It helps tremendously when you later on try to understand what things are doing. I actually have a dictionary and a thesaurus right next to my computer, just for that.

- Robert

Groby
Friday, September 19, 2003

Your SetCoords doesnt make sense, it locates and set the dimensions of the shape.  If you are trying to place a shape in a particular place, then the shape should already have dimensions, if it doesnt have dimensions, then its not a shape. So then you can just place the center.  Assuming rotation is supported:

SetCoords( ICoord center, IAngle orientation );

IShape's can be moved around and rotated.

B
Friday, September 19, 2003

Totally agree that setCoords should take different arguments for each shape. The only alternative is a parallel heirarchy of classes to handle the coords data, or something like

setCoords(vector<coord>)

where the interpretation of the vector contents is dependant on what shape you are dealing with. The first means you are writing way too much code; the second means you are throwing away some compile-time type-checking and opening yourself up to a whole new class of errors.

David Clayworth
Friday, September 19, 2003

Folks, remember this is a contrived question. Getting stuck on an ICoord is silly. It could be pink elephants, the problem is the class architecture, not trying to implement a drawing package.

Peter Ibbotson
Friday, September 19, 2003

Hey Grumpy Old-Timer,

Would it work to replace the shape with a new one at a different location, instead of modifying it in place? That way, you can use the constructor to specify the type-specific co-ordinates.

Ken Dyck
Friday, September 19, 2003

I think the problem here, as has been mentioned, is that the OP is trying to set properties on objects polymorphically, and that doesn't really make sense in this context.  What you need to look at, I think, is what you are actually trying to *do* when you are setting these properties.  People have made a number of suggestions that relate to shapes... maybe you are trying to translate, or rotate.  You will have to determine the actions that make sense for your classes.  The point is that you should try to change your "set properties polymorphically" to "do an action polymorphically, a side effect of which might be to change properties".

Mike McNertney
Friday, September 19, 2003

"If Polygon stores a list of line segments, would you have an infinite list?  It seems like you are falling into the trap of trying to capture the "real world" as described in the C++ FAQ link."

Andy I didn't mean circle and polygon to be represented infinite lines. Nor did I read the C++ FAQ. I was just thinking how Grumpy Old-Timer's calling convention can be solved. What I can think of is a different implementation of setcoords interface for circle and ellipse subclasses as the interface can be a same.

And in graphics how do you draw a circle. I think its a bunch of moveto's and lineto's. So I"m not so off the target. But storing them is really insane !!!. Any way my point was to make the interface look good.

cooler
Monday, September 22, 2003

Grumpy,

If you are still reading this thread, did ya ever get your problem worked out?  What strategy did you select?  Just curious...

anon
Monday, September 22, 2003

The real question is, what do you mean by shape?  If you mean, a visually distinct geometric object, then you're in trouble.  If you mean an object that will emit graphics commands dependant on its internal state, then you may be onto something.

In the second case, you can say that you can limit where they will emit these commands.  Say a SetBounds(Rect) method.  A circle will just take min(dx, dy) as its diameter, and use the center as its own.  An ellipse is even better.  If you want a SetRotation, then you're going to have to convert that bounding rectangle appropriately.

Whenever you're having problems with a question like this, make sure you're asking the right question :-)

H. Lally Singh
Tuesday, September 30, 2003

*  Recent Topics

*  Fog Creek Home