5
\$\begingroup\$

I've created a rudimentary example of the issue I'm inquiring about: Object slicing. I followed a post on Stack Overflow which provided a similar template - and for all intents and purposes, this works. I was just wondering if this would get me into a trouble longer down the line? Is there anything here that jumps out at you as dangerous and/or wrong?

The heart of the logic is in main(). I'm pushing a Circle object into a Shapes* vector, then push the new Shapes* element into a Circle vector. The issue I had WITHOUT the assignment operators, was that the compiler couldn't decide how to convert Shape* to Circle.

Please let me know if there are any glaringly obvious improvements.

class Shape {
public:
    virtual Shape& operator=(const Shape& s) {
        assign(s);
        return *this;
    }

    virtual std::string getName() = 0;
    virtual         int getEdges() = 0;



protected:
    std::string name;
    int         edges;

    void assign(const Shape& s) {
        this->name = s.name;
        this->edges = s.edges;
    }
};

class Circle : public Shape {
private:
    int radius;
public:
    Circle() { name = "Circle"; edges = 1; }
    Circle(int rad) { name = "Circle"; edges = 1; radius = rad; }

    virtual Circle& operator=(const Shape& s) {
        if (const Circle* c = dynamic_cast<const Circle*>(&s))
            assign(*c);
        else{
            std::cout << "BAD ASSIGNMENT IN CIRCLE.";
                    //THROW ERROR HERE INSTEAD OF THE ABOVE COUT
            }
        return *this;
    }

    std::string getName() { return name; }
            int getEdges() { return edges; }
            int getRadius() { return radius; }
           void setRadius(int r) { radius = r; }

protected:
    void assign(const Circle& c) {
        Shape::assign(c);
        this->radius = c.radius;
    }

};

int main() {
    std::vector<Shape*> shapes;
    std::vector<Circle> circs;
    Circle c2(5); //Creates a circle with 5 for the radius.

    shapes.push_back(&c2); //Pushing the 5-radius circle into the Shapes* vector
    Circle c3; //Creates a circle with default constructor (which does NOT define radius)
    c3 = *shapes[0]; //Now, the overloaded assignment operator. Look at Circle::assign(const Shape&) function
    circs.push_back(c3); //We push our newly assigned circle to our Circle vector
    std::cout << "c3 radius: " << circs[0].getRadius(); //This will be 5!
}
\$\endgroup\$

3 Answers 3

9
\$\begingroup\$

Allowing a Shape to be assigned to a circle does not sound like it is very safe. I suppose there are situations where it can be used but I would advice against it in the general case

Also I would use Curiously Recurring Template Pattern to get around having to write the assignment operator-er in every derived class.

class Shape
{ // STUFF
};

template<typename T>
class ShapeCopyable: public Shape
{
    T& operator=(const Shape& s)
    {
        T const& c = dynamic_cast<T const&>(s);  // Throws on bad cast.
        assign(c);

        return *this;
    }
};
class Circle: public ShapeCopyable<Circle>
{
    // Stuff
};

In addition to what @Chris Jester-Young covered (so everything he said):

The Base class of a hierarchy with virtual methods should also have a virtual destructor. Otherwise you are going to start having issues with memory management when you start dynamically creating objects on the fly.

class Shape {
public:
    virtual ~Shape() {}

Don't define the copy constructor or assignment operator if nothing special needs to be done. Let the compiler generated version do the work for you. Unless you have something special in the destructor or have RAW pointer members this is unlikely. PS you should not have RAW pointer members either.

    // Remove this:
    virtual Shape& operator=(const Shape& s);

Don't like getters/setters. They break encapsulation and expose the internal implementation of the object. You should consider why you need to pass the state of the object out (is the function that uses the values really a member function).

Interactions on the object should usually be done internally to the object. This will get rid of most of your needs for getters and setters. If you still have a need after the analysis fine.

    // Don't like them.
    // Make but sometimes you need them but minimze your
    // exposure by using methods that manipulate the object rather
    // than moving the manipulation to an external actor.
    std::string getName() { return name; }
            int getEdges() { return edges; }
            int getRadius() { return radius; }
           void setRadius(int r) { radius = r; }

A vector of pointers. Red flag who owns the pointers.

    std::vector<Shape*> shapes;

A'hh you are pushing stack objects onto the vector.

    shapes.push_back(&c2);

Maybe it should be a vector of reference wrapper to Shape std::vector<std::reference_wrapper<Shape>>.

Getting a circle from a shape.

    c3 = *shapes[0];

Not sure that should really happen. The interface to Shape should be how you manipulate the object. Needing to convert the object to a circle seems unnecessary in most situations (you should probably have virtual functions that do the actions you need). Sometimes things have unique actions then you need to convert them back.But in these situations just use dynamic_cast directly (this also shows you are doing the conversion very explicitly).

Based on comments below:

I will need to pull name's and description's to display to the user

Its easier to ask the object to display itself rather the pull details about the object and then display them.

The simplist example is just printing information to a stream:

// Original Way
Circle    c(5);
std::cout << "Circle: "  << c.getName() 
          << " Edges: "  << c. getEdges 
          << " Radius: " << c.getRadius()
          << "\n";

// What I would do is make the serialization of the object to a stream
// a member method that does the work.

class Shape
{
    virtual void print(std::ostream& stream) const = 0; // Can print a Shape
                                                  // Must be specialized for 
                                                  // Specific types.

    friend std::ostream& operator<<(std::ostream& stream, Shape const& shape)
    {
        shape.print(stream);
        return stream;
    }
};

// Now you print like this:
Circle    c(5);
std::cout << c << "\n";

You don't need to expose the members just because you need serialization. Doing it this ways adds consistency so that all objects of a particular type are serialized in the same way. If you need to change what it looks like you have a single place in the code that needs modifying.

We can extend the above to other graphical representations.

struct DisplayInterface
{
     // This is an interface for drawing shapes on a display.
     // You can imagine it has the standard drawing functions.
     // But imagine a base set.
     void drawLine(Pt const& tr, Pt const& bl);
     void drawCircle(Pt const& tr center, unsigned int radius);
     void drawPolygon(std::vector<Pt> const& pts);
     void drawText(Pt const& bl, unsigned int fontSize, std::string const& str);
};

// Now in your drawing code you pass the DisplayInterface to the object
// So it can draw itself. So usually the drawing system provides an
// interface that all drawable objects need to implement.

struct Drawable
{
    virtual void display(DisplayInterface& di) = 0;
};

class Shape: public Drawable  // All shapes are drawable
{                             // But each shape must implement
                              // Its own display()
};

class Circle: public Shape
{
     virtual void display(DisplayInterface& di)
     {
          di.drawCircle(Pt(0,0), radius);
          di.drawText(Pt(0,15), 12, name);
     }
};
\$\endgroup\$
13
  • \$\begingroup\$ Apologies for the late reply, but thank you very much for the informative and thorough response. I'm not sure I like the implementation either, but here's my reasoning (in line with the Shape analogy): My project doesn't contain just Circle's, but a plethora of other shapes (Triangle's, Square's, etc). The user will be in control of X number of ShapeFactory's, which construct Circle's, and other classes derived from Shape. Each Shape has a different construction length. \$\endgroup\$ Jun 18, 2014 at 23:52
  • \$\begingroup\$ Construction length is in turns (as it is a turn-based game). So, instead of having vector's for Circle's, and all the different derivations to keep track of construction, I push all the derived shapes into a vector<Shape*>, and every turn, I simply cycle through the vector and decrement everything that still has turns remaining for construction. When it's done construction, it is pushed to the appropriate vector<Circle> or vector<Square>, etc, to keep track of the Shape's that are ready for the user to use. (Each Shape has an ID, so that I can push it to the appropriate vector. \$\endgroup\$ Jun 19, 2014 at 0:00
  • 1
    \$\begingroup\$ Quote: I will need to pull name's and description's to display to the user. I think that is a bad idea. Pass an interface to object that does the display then let the object decide what is displayed. void Shape::display(DisplayInterface& di); \$\endgroup\$ Jun 19, 2014 at 7:37
  • 1
    \$\begingroup\$ This is where I would be pushing Circle objects to the construction queue (the vector<Shape*>), and push them to the vector<Circle> when they are constructed. Is there a smarter way of approaching this? Yes. Just keep a vector of shape. It seems unlikely you need to differentiate then later when you don't earlier. \$\endgroup\$ Jun 19, 2014 at 7:39
  • 1
    \$\begingroup\$ @JhomasTefferson: See extension to answer. \$\endgroup\$ Jun 19, 2014 at 15:42
5
\$\begingroup\$
  1. Initialise member variable outside the body of the constructor. e.g.,

    Shape(string const& name, int edges) : name(name), edges(edges) {}
    Circle(int radius) : Shape("Circle", 1), radius(radius) {}
    
  2. The getName function should return by const reference (since it's actually backed by a real value in your class), not by value. That way, the string doesn't have to get copied. (On the other hand, returning by value is appropriate if you're returning a temporary object that isn't backed by anything.)

  3. All the getter functions should be declared const. e.g.,

    virtual string const& getName() const;
    virtual int getEdges() const;
    
  4. Since the name and edges properties are inherent to Shape, the implementation of getName and getEdges should live there, not in Circle. Thus, they should not be pure virtual.

  5. Consider creating two overloads for Circle::operator=, one that takes a Shape const& (and does the dynamic_cast) and one that takes Circle const&. That way, when it's known at compile-time that you're passing in a Circle, you skip the dynamic_cast. (Some C++ programmers consider RTTI to be expensive and when you can avoid RTTI easily, like here, it's probably worth doing.)

  6. Since you were planning to throw an exception if it weren't a Circle, you can use the reference form of dynamic_cast. You can catch-and-rethrow (throw; with no arguments does a rethrow) if you want to do some logging in between.

  7. Why have a separate assign function? Your operator= is already virtual, you should do all the work in there.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you very much! I've implemented some of these suggestions already! (Daunting with 50ish .cpp and .h files!) Just a quick question: Should I be returning everything as const references? Even int returns? ie, const int& getID();? Would referencing the smaller data types make a difference? Furthermore, should I do this with my vector's? (ie, const std::vector<Shape>& getShapes()). Or would it be better to write a copy constructor for Shape? \$\endgroup\$ Jun 19, 2014 at 0:04
5
\$\begingroup\$

Is there anything here that jumps out at you as dangerous and/or wrong?

The problem you are trying to solve is the x-y problem. You are trying to treat circles (a specialized class) in a generic matter, without having the public API of Shape reflect that.

The correct solution is to add the operations that apply to the hierarchy to the shape class, not hack them on, down the hierarchy.

Usually, when you need to use dynamic_cast in your code, that's a sign that you're defining or using your class hierarchy wrongly (i.e. the problem is either the API of the class hierarchy not being generic enough, or the way it is used in client code (client code works on shapes when it should work on circles instead); adding a dynamic_cast fixes/avoids the problem, while at the same time increasing module coupling, reducing modularity and introducing the need for future boilerplate code).

Conceptually also, assigning a generic shape to a specialized shape doesn't make sense.

You could categorize your properties into facets/aspects ("these shapes have an area", "these shapes have a gravitational center", etc) and decide which facet each specialization should support/copy/etc. Then, you could:

  • add a generic facet API to Shape's public interface
  • support whatever facets you need to introduce in a generic way
  • add facet specializations as needed, down the hierarchy (and othogonal to it)
  • get rid of the dynamic_cast
  • marry, have 2.5 kids and a dog, and live happily ever after

I was just wondering if this would get me into a trouble longer down the line?

It introduces a (bad) implementation dependency into the entire class hierarchy. Consider that you pass your classes to another developer and she wants to add a specialization to Circle:

class ColoredCircle: public Circle
{
public:
    // you are forced to implement this to avoid ColoredCircle slicing
    virtual ColoredCircle& operator=(const Shape& s) {
        if (const ColoredCircle* c = dynamic_cast<const ColoredCircle*>(&s))
            assign(*c);
        else{
            throw omg_exception("!");
        }
        return *this;
    }

protected:
    void assign(const ColoredCircle& cc) {
        Circle::assign(cc);
        this->color = cc.color;
    }

private:
    SomeColorType color;
};

This works.

If you have five specializations though, you get lots of duplicated code. More than this, you are now forced to propagate dynamic_cast calls through the entire hierarchy (your one-time use of dynamic_cast is now endemic).

You can ease this by implementing a CRTP implementation for the assignment operator ( see LokiAstari's answer ).

Other observations:

std::vector<Shape*> shapes;
std::vector<Circle> circs;
Circle c2(5); //Creates a circle with 5 for the radius.

shapes.push_back(&c2); //Pushing the 5-radius circle into the Shapes* vector

Here, you are adding the address of an automatic variable to the vector ( I assume this is just SCCCE client code, but still :) ). The object at that address is invalidated before the vector (it works, but the dependency is not explicit).

\$\endgroup\$
9
  • \$\begingroup\$ What I wanted to say but done more eloquently, \$\endgroup\$ Jun 17, 2014 at 15:42
  • \$\begingroup\$ I replied to Loki's comment with this to clear up my lgoci: There are X number of ShapeFactory. Each ShapeFactory will need to keep track of which derived-from-Shape classes it "constructs" (not in the programming sense). So, each ShapeFactory would have its own vector<Shape*> constrQueue, and its own vectors of Circle's, etc, to keep track of "constructed" and ready-to-use shapes. This is where I would be pushing Circle objects to the construction queue (the vector<Shape*>), and push them to the vector<Circle> when they are constructed. Is there a better way of approaching this? \$\endgroup\$ Jun 19, 2014 at 0:39
  • \$\begingroup\$ instead of having vector's for Circle's, and all the different derivations to keep track of construction, I push all the derived shapes into a vector<Shape*>, and every turn, I simply cycle through the vector and decrement everything that still has turns remaining for construction. When it's done construction, it is pushed to the appropriate vector<Circle> or vector<Square>, etc, to keep track of the Shape's that are ready for the user to use. \$\endgroup\$ Jun 19, 2014 at 0:40
  • \$\begingroup\$ I feel like the only solution to accomplish this is to have separate vector<Circle> circConstr; and vector<Square> sqConstr;, etc, to keep track of all the differing "construction queues". It will render the reason behind my base Shape class as useless. \$\endgroup\$ Jun 19, 2014 at 0:43
  • \$\begingroup\$ You say "The object at that address is invalidated before the vector (it works, but the dependency is not explicit)." at the end, could you expand on that? What invalidates it? Why is the dependency not explicit (and for a double-dose of honesty, what makes dependency explicit/implicit? Could you provide a thread/question that explains it? \$\endgroup\$ Jun 19, 2014 at 0:47

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.