2

I am writing a scientific code which needs to create 3-dimensional cells, defined by a set of faces, which are defined by a set of vertices.

These 3 classes (Cell, Face, Vertex) are derived respectively from some generic geometry classes (Polyhedron, Polygon, Point) which implement some geometric routines like Polygon::CalculateArea().

The Face class adds to the Polygon class with additional data and functions required for the science, like Face::Interpolate(). I don't want to make these member functions virtual in the base class (Polygon).

Now, the problem. I initialize a Cell with a vector of pointers to Face, which is handled by the base class Polyhedron constructor, which upcasts the Face* to Polygon*:

Polyhedron::Polyhedron( std::initializer_list<Polygon*> polygons );

Later, I want to access the Face* stored in a Cell so that I can call Face::Interpolate(), but it has been stored as a Polygon* and thus has no member function Polygon::Interpolate(). I can downcast it manually back to a Face* which works, but is not very clean. The user of the code has to do something like:

Face * temp_face = (Face*)cell->GetFaces()[0]; // Could use static_cast
temp_face->Interpolate();

which is not obvious.

I want the interface to be transparent, so that this just works:

cell->GetFaces()[0]->Interpolate();

I can think of two or three ways to achieve this. I'm looking for a better solution or feedback of which of these is recommended:

  1. In Cell::GetFaces() which currently just inherits from Polyhedron::GetPolygons() I could create a wrapper that copies the std::vector<Polygon*> to a new vector std::vector<Face*>. This seems sloppy to me, not easy to maintain, inefficient and prone to errors.

  2. Instead of storing std::vector<Polygon*> I could store std::vector<std::shared_ptr<Polygon>>. From what I understand, these smart pointers retain type-awareness so that they can call the right destructor, but they might just store a reference to the destructor depending on implementation. I don't want to use shared_ptr for performance purposes -- I know they're good and friendly, but I'm creating millions of these Polygons and its easy to destroy them in the right place. I can't use unique_ptr easily because of the copy-constructor used in std::initializer_list constructors.

  3. Template the whole Polyhedron class, replacing every instance of Polygon* with F* and checking that F is a base of Polygon:

    template<typename F = Polygon>
    typename std::enable_if<std::is_base_of<Polygon, F>::value, void>::type
    class Polyhedron
    

    and then inheriting from a parent with a given typename:

    class Cell : public Polyhedron<Face>
    

    This seems like the best method to me, since it has the least boilerplate and nothing exposed to the user; but it still feels messy, especially in the "real" case where there might be multiple types that would all have to be specified:

    class Cell: public Polyhedron<Face,Vertex,type3,type4,type5,...>
    

Is there a a better way? Perhaps a means of retaining type in the original vector (or some other container)?

If not, which of the above methods is the best practice and why?

Edit: Here's an abstracted view of the problem. The problem occurs when trying to run sumOfSomethingSpecific(). In my actual problem, that function is inside a derived class Derived_B, which is designed to work with Derived_A, but for the sake of the problem, it makes no difference.

class Base_A
{
public:
    Base_A();
    ~Base_A();
    // I don't want virtual doSomethingSpecific() here.
};

class Derived_A
{
public:
    using Base_A::Base_A;
    double doSomethingSpecific();
};

// I could template this whole class
// template <typename T>
// where T replaces Base_A
class B
{
public:
    // This can be initialized with:
    // std::vector<Derived_A*>
    // which is what I want to do, but we lose info about doSomethingSpecific()
    // even if I write a separate constructor its still stored as
    // std::vector<Base_A*>
    B(std::vector<Base_A*> v) : v(v) {};
    ~B();
    double sumOfSomethingSpecific()
    {
        double sum = 0;
        for(auto&& A : v) {
            // Can't do this, A is a pointer of type Base_A*, but this is the abstraction that I want to achieve
            sum += A->doSomethingSpecific();
            // Could do this, but its ugly and error-prone
            Derived_A* tempA = (Derived_A*)A;

            sum += tempA->doSomethingSpecific();
        }
        return sum;
    }
protected:
    std::vector<Base_A*> v;
};
James
  • 91
  • 6
  • Huh? A `Face` is not a `Polygon`. I think your problems start there. – Barry Jun 08 '15 at 14:26
  • Face inherits from Polygon: `class Face : public Polygon`. Is there a problem there? – James Jun 08 '15 at 14:33
  • Yeah, that logically doesn't make any sense. A polygon has faces, edges, and vertices. A face is not a polygon. – Barry Jun 08 '15 at 14:40
  • @Barry, I desagree, from wikipedia: *In geometry, a polygon is traditionally a plane figure that is bounded by a finite chain of straight line segments closing in a loop to form a closed chain or circuit. These segments are called its edges or sides, and the points where two edges meet are the polygon's vertices (singular: vertex) or corners.* There is no way a 2d figure has faces. – Raydel Miranda Jun 08 '15 at 14:51
  • @Barry I see what you mean, maybe I will rename some of my geometry classes. Here I am using `Point`, `Polygon` and `Polyhedra` as 0D, 2D and 3D "things" in 3D space. You're right that logically it makes sense to construct a Polyhedra out of Faces, rather than Polygons as I have done -- but that's a naming problem; doesn't change my casting/inheritance problem. – James Jun 08 '15 at 14:56
  • @James I might be missing something, but if it is only to make the interface transparent, you can you add a method `const Face &GetFace(size_t) const` to the `Cell` to implement the cast once for all? In that case, `cell->GetFace(0).Interpolate()` would just work and the users won't need to know how the faces are stored. You could even make the inheritance protected to avoid confusion (Polyhedron seems to be an implementation detail of the representation of a Cell) – Come Raczy Jun 08 '15 at 20:35
  • That's not a bad idea @Come Raczy. What if I wanted to return a vector though (i.e. GetFaces())? – James Jun 08 '15 at 20:52
  • @James If you want to return a vector you have several options, the first one being to change your mind and carefully reconsider your actual needs. If your need is only to easily iterate over all the faces, you could consider providing an iterator for that purpose (boost iterator might have what you need). If you really need random access to the faces, the simple, safe and maintainable (but slow and wasteful) option is probably to implement something like `std::vector GetFaces() const;` that creates the vector on each call and delegates the conversion to the `GetFace(size_t)`. – Come Raczy Jun 15 '15 at 16:44

2 Answers2

2

First most of issues you're facing here are not about programming, are about design.

... class with additional data and functions required for the science, like Face::Interpolate(). I don't want to make these member functions virtual in the base class (Polygon). ...

Well, don't do that, but then you have to realize that you're adding complexity to the code you need to implement such design desicion.

However, if every polygon can be "interpolated" then you should have a virtual function (or better yet a pure virtual function) in your Polygon class.

Said that, with the code as it is, in order to add transparency to the API you declare you get_* functions as:

void GetFaces(std::vector<Face *> &faces);

that way is clear for the user that he/she has to provide a reference to a vector of faces to get the result. Lets see how this change your code:

// Face * temp_face = (Face*)cell->GetFaces()[0]; // Could use static_cast
std::vector<Face *> temp_faces;
cell->GetFaces(temp_faces);
//temp_face->Interpolate();
temp_faces[0]->Interpolate();

This way the down-cast is performed implicitly.

About your question: Is there a a better way? Yes, redesign your classes.

About your example:

I will ask you to think a moment about this:

struct Base {};
struct Derived_A: Base { double doSomethingSpecific(); };
struct Derived_B: Base { double doSomethingSpecific(); };

int main()
{
    std::vector<Base*> base_v = {/*suppose initialization here*/};

    base_v[0]->doSomethingSpecific();  // Which function must be called here? 
                                       // Derived_A::doSomethingSpecific or
                                       // Derived_B::doSomethingSpecific.
}

At some point you will have to tell wich type you want call the function on.

The level of abstraction you want, does not exists in C++. The compiler needs to know the type of an object in order to perform (compile) a call to one of its member functions.

Another approach you can try (I still recommend to redesign):

If you have the need of manipulating several distinct types in a uniform manner. Perhaps you want to take a look at Boot.Variant library.

Raydel Miranda
  • 13,825
  • 3
  • 38
  • 60
  • Thank you. The reason for the complexity is because if someone wants to write a new `class Face2 : public Polygon` in the future, with specific functions to handle whatever that `Face2` needs to do that's so special, I want that person to be able to implement that class without adding virtual functions to the original `Polygon`. Maybe this is wrong. The solution to pass an arg_out is nicer than what I have (at least there's some type-checking) but still not perfect. I assume the compiler can't optimize-away the pointer copying, however, which I want to avoid. – James Jun 08 '15 at 15:39
  • @James: You cannot avoid it in the general case. It can only be avoided given a very narrow set of circumstances, which can't be ensured portably and can't be ensured unportably without knowing the definition of every possible derived class and intrusively preventing them from doing lots of legal things. – Puppy Jun 08 '15 at 15:54
  • @James, if someone wants to write a special Face, lets call it `SpecialFace` he/she should implement a `SpecialPolygon`. There is no reason a "standar" Polygon have specials faces. Again you're dealing with conceptual problems. Try to read something about over-engineering. – Raydel Miranda Jun 08 '15 at 15:55
  • I've updated the question with a minimal example, perhaps it helps explain the problem. – James Jun 08 '15 at 18:03
  • Thanks for your additions @RaydelMiranda. Your question "Which function must be called here?": Basically I want to infer this from the initialization routine. Obviously in our examples you can't because its just initialized with . I could template this initialization, so that I have a version for Type=Base* and Type=Derived_A* and Type=Derived_B*. I could even static_assert in relevant places to ensure that the Type has a function doSomethingSpecific(), and produce compile messages if not: http://stackoverflow.com/a/25216349/1965265 I think that works.... – James Jun 08 '15 at 21:03
  • However, I am considering re-designing, this seems very messy already. – James Jun 08 '15 at 21:04
-1

I struggled with a similar problem in one of my projects. The solution I used was to give ownership of the actual objects to the most-derived class, give the base class a copy of the objects, and use a virtual function to keep the copy up-to-date as objects are added/removed:

class Polyhedron {
protected:
    bool _polygons_valid = false;
    std::vector<Polygon*> _polygons;
    virtual void RebuildPolygons() = 0;
public:
    std::vector<Polygon*>& GetPolygons()
    {
        if (!_polygons_valid) {
            RebuildPolygons();
            _polygons_valid = true;
        }
        return _polygons;
    }

    /*Call 'GetPolygons()' whenever you need access to the list of polygons in base class*/
};

class Cell: public Polyhedron {
private:
    std::vector<Face*> _faces;  //Remember to set _polygons_valid = false when modifying the _faces vector.
public:
    Cell(std::initializer_list<Face*> faces):
        _faces(faces) {}

    //Reimplement RebuildPolygons()
    void RebuildPolygons() override
    {
        _polygons.clear();
        for (Face* face : _faces)
            _polygons.push_back(face);
    }
};

This design has the benefits of clear ownership (most-derived class is owner), and that copying and upcasting the vector of object pointers is done only when needed. The downside is that you have two copies of essentially the same thing; a vector of pointers to objects. The design is very flexible too, since any class derived from Polyhedron only has to implement the RebuildPolygons() function, using a vector of any type derived from Polygon.

Carlton
  • 4,217
  • 2
  • 24
  • 40
  • This is very bad advice. You are basically suggesting not to use polymorphism, because you can use branching (`if (_polygons_valid) { ... } else { ... }`). Also, you're coding a variant, but manually. So, a `boost::variant` would be superior. In fact, of course `BaseVector` in and off itself is superior. Lastly, nothing about those ownership semantics has improved. Instead, it has been complicated because there are now always non-owning references which could go stale. – sehe Jun 08 '15 at 19:52
  • @sehe what if I later need a `Derived3Vector`? Do I then refactor every class that uses the `boost::variant`? That seems very brittle. – Carlton Jun 08 '15 at 20:12
  • It's _more_ brittle to keep adding the branches manually. Also, the obvious solution is to either not use polmorphism in the first place, or to accept it (and hence use pointer-to-base). It's really not too complicated that way. – sehe Jun 08 '15 at 20:23