Is this bad? (How can this be avoided?)

Discussion in 'C++' started by mike3, Nov 28, 2011.

  1. mike3

    mike3 Guest


    Is this bad? Suppose you have something like this:

    class Shape {
    ... blah ...

    class Rectangle : public Shape {
    ... blah ...

    class Circle : public Shape {
    ... blah ...

    .... blah ...

    class Drawing {
    ... blah ...
    std::vector<Shape *> shapeList;
    ... blah ...
    ... blah ...
    void addShape(Shape *s)
    ... blah ...
    void doSmth(); // uses the objects in shapeList somehow
    ... blah ...

    There could arise a situation like this:

    void someFunction()
    Drawing drawing;

    ... blah ...
    Circle circle(35.0, 55.0, 25.0); // make a circle & (35, 55)
    // radius 25
    drawing.addShape(&circle); // add it to the drawing
    // OOPS! circle goes out of scope...!

    ... blah ...

    drawing.doSmth(); // BOOM!

    ... blah ...

    What to do? Now one could, I suppose, change to Circle* circle = new
    Circle(35.0, 55.0, 25.0);, but then we need something to take care of
    the garbage. Adding a delete() in, say, the destructor for Drawing
    might work, but we make some assumptions that may or may not be true,
    i.e. one can break it by misusing it, e.g. if one does not use a new
    to get the parameter or one forgets that destroying drawing also
    destroys circle, etc. Is it a bad idea to rely on the user's
    understanding -- to me it doesn't seem so. What should be done?
    mike3, Nov 28, 2011
    1. Advertisements

  2. mike3

    mike3 Guest

    And should the function addShape() be upgraded to use shared_ptr?

    And also, though this solves the "garbage collection" issue, it still
    the user needs to understand that they must pass an object that's on
    the heap,
    and that it will be lost when the Drawing object is gone. Is that OK?
    mike3, Nov 28, 2011
    1. Advertisements

  3. mike3

    Ian Collins Guest

    Most drawing libraries I've used work this way, the container "owns" the
    drawable objects. The simplest solution is to have the Drawing
    destructor free all of the Shape objects it owns. So they all assume
    the drawable objects are on the heap.

    If you want something more complex, such as the same shape in more than
    one drawing, then something like a shared_ptr is the solution.
    Ian Collins, Nov 29, 2011
  4. mike3

    mike3 Guest

    So then the requirement of the user to have to explicitly heap
    otherwise bad things could happen is okay?
    mike3, Nov 29, 2011
  5. mike3

    MikeWhy Guest

    After the benefit of many years of occasionally revisiting ye olde taxonomy
    on geometry problem, I offer the following as general advice and specific
    comment for this particular question.

    For polymorphic heirarchies, the base class defines how the outside world
    interacts with the concrete sub-types. Its role and content deserve much
    more thought and effort than simply "...blah...", before diving immediately
    into the derived types. Doing so places the proverbial cart before the
    horse, as it were.

    Further, almost all "solutions" miss the key abstractions, focusing instead
    on the relatively meaningless distinction between rectangles, circles, and
    pentagons, ad nauseum. The vertex-list, or vertex generation, seems implicit
    in every naive implementation, but I've seen little effort to bring it
    forward as an explicit, exploitable concept.

    Last, the more important distinction from my point of view is the difference
    between a spline, and a haphazard shape connected by straight line segments.

    On the other hand, if ye olde professor truly did intend for you to focus on
    the trivialities of four-sided versus five-sided regular geometric shapes,
    stick with his program, however much you might feel above this simplistic
    learning. The larger danger is in over-engineering the solution and still
    missing the simplistic mark that was placed before you.


    class Shape2D {
    Shape2D(Vertex2D originXY);
    Shape2D(Vertex2D originXY, Vertex2D scaleXY);
    Shape2D(Vertex2D originXY, Vertex2D scaleXY, Scalar rotation);

    // Shape2D(const Shape2D &); // default copy is appropriate.
    void Draw(Canvas2D &) const;

    // void SetColor(Color); // as needed

    virtual DrawTo(Canvas2D &) const = 0; // abstract

    Vertex2D origin;
    Vertex2D scale;
    Scalar rotationXY;
    VertexGen & vertices;
    MikeWhy, Nov 29, 2011
  6. mike3

    Ian Collins Guest

    It's certainly not uncommon. See the Qt documentation for an example.
    Ian Collins, Nov 29, 2011
  7. mike3

    Goran Guest

    To state the obvious... Your question is relatively unrelated to your
    example. It's about probably the most important aspect of C++
    programming practice: who or what OWNS objects that are in your code.
    IIRC, Stroustroup (and standard? dunno) talk about storage classes:

    1. static: objects created at startup (before main) and destroyed at
    exit (after main)
    2. automatic: objects created inside a block (any {} is a block)
    3. dynamic: objects created using new

    For 1 and 2, owner is language runtime. For 3, owner is the code
    itself, or the programmer. So you, the programmer, have to make rules
    and obey them to the letter. That includes your users.

    There's NO WAY, whatsoever, to avoid that.

    For example, Leigh said use shared_ptr. That's a reasonable advice,
    but you or your user still MUST obey the rule that shared_ptr owns the
    object and must NOT, ever, do e.g. Shape s; shared_ptr<Shape>(&s);. Of
    course, with a shared_ptr, or other smart pointers, it's such a common
    knowledge that you can only create them with heap objects, which makes
    it a good idea to always use smart pointers for heap objects.

    So... You need to make a decision and stick to it. If you e.g. decide
    to "take ownership" of a Shape in addShape, it's possibly best to make
    it "void addShape(auto_ptr<Shape>)" (or unique_ptr) and benefit from
    aforementioned common knowledge. Your users are at fault if they don't
    use knowledge ;-).

    Goran, Nov 29, 2011
  8. mike3

    Joe keane Guest

    Maybe you want

    std::vector<Shape> shapeList;

    Joe keane, Nov 30, 2011
  9. mike3

    MikeWhy Guest

    Direct containment doesn't work for polymorphic heirarchies.
    MikeWhy, Nov 30, 2011
  10. mike3

    Joe keane Guest

    'engage brain before running mouth'
    Joe keane, Nov 30, 2011
    1. Advertisements

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments (here). After that, you can post your question and our members will help you out.