0

I have a class called Shape. Shape contains an instance of a Geometry class called geometry and a pointer of type vector. The Geometry class has a variable called vertices of type vector.

Instead of assigning vertices to my shape like:

Shape* myShape = new Shape;
myShape->geometry->vertices = vertices;

I would like to do:

Shape* myShape = new Shape;
myShape->vertices = &vertices.

Why? Because it looks nicer. Could I do this with a function? Yes, but I don't want to. To achieve this, in Shape's constructor, I set it's pointer to the address of geometry's variable.

Shape::Shape()
{
    geometry = new Geometry; // have also tried Geometry()
    vertices = &(geometry->vertices); // vertices declared as vector<vec3>* vertices in header
}

Except when I assign vertices to myShape->vertices they don't get assigned to myShape->geometry->vertices. It seems like they get assigned to myShape->vertices still. What gives? How can I connect a pointer to my instanced class, classes member and cut out the middle man?

Stradigos
  • 814
  • 1
  • 13
  • 29
  • 3
    You should not expose class fields (make them public). Why not create `setGeometry` method in Shape class instead (which is trivial to implement, by the way)? –  Jan 26 '15 at 22:10
  • 2
    Creating a new `Geometry` object inside the constructor and setting `verticies` to it is not correct. You should have a `Geometry*` as a data member instead. – David G Jan 26 '15 at 22:30
  • @deniss, my thought was that if I need to create a get/set for a private variable, why not just make it public? Seems like a lot of work for nothing. Great thread on this: http://stackoverflow.com/questions/737409/are-get-and-set-functions-popular-with-c-programmers – Stradigos Jan 26 '15 at 22:38
  • @0x499602D2, I'm sorry, that was a typo. I am not correcting a new Geometry object in there. Good catch. I do in fact have a Geometry* defined in the header. – Stradigos Jan 26 '15 at 22:40

2 Answers2

2

Since Shape::vertices is a pointer you are just rebinding it to a different object when you do 

myShape->vertices = &vertices

you are not copying vertices into Geometry::vertices as you have figured out.

While I do not agree with your approach (it violates, among several other things, the open/closed principle), you can achieve what you wanted by returning a reference to Geometry::vertices instead.

private:
Geometry geometry;

public:
vector<vec3>& Shape::vertices() { return geometry->vertices;}

And use:

myShape->vertices() = vertices;

A better strategy:

  1. Avoid unmanaged resource acquisition in constructors. You are acquiring an instance of Geometry object in the constructor and assigning it to a raw pointer. This could lead to problems, e.g. what would happen to that object if your constructor fails? In general, it is a good habit to observe RAII. That would be by using smart pointers in your case.

In code:

Shape::Shape(): geometry(new Geometry()) {}
private:
std::unique_ptr<Geometry> geometry;
  1. Do not expose member variables: I don't think it is ever a good idea to expose the data members of a class let alone a pointer data member. By exposing data members 1) you are requiring the user knowledge of your class to go beyond what your class does to how it is implemented. 2) you are closing the door to future changes in your internal implementation without refactoring all your class users codes. 3) your class members are often invariant of that class, by exposing them users will potentially break things down.

To sum up, I would redesign your code in this way:

class Geometry
{
  private:
    std::vector<vec3> vertices;
  public:
    void addVertex(vec3& vtx) { vertcies.push_back(vtx);}
    void addVertices(std::vector<vec3>& vtxs) { for(auto& vtx:vtxs){ vertices.push_back(vtx);}}
}

class Shape
{
  private:
    std::unique_ptr<Geometry> geometry;

  public:
    Shape(): geometry(new Geometry()) {}
    void addVertex(vec3& vtx) { geometry->addVertex(vtx);}
    void addVertices(std::vector<vec3>& vtx) { geometry->addVertices(vtxs);}
}

P.S. I assumed that your Geometry::vertices is of type vector<vec3> as implied in your question..

Mustafa
  • 1,814
  • 3
  • 17
  • 25
  • Aren't getters and setters evil though? If I'm not doing anything more interesting with the data, why not just expose it? Interesting thread here: http://stackoverflow.com/questions/737409/are-get-and-set-functions-popular-with-c-programmers – Stradigos Jan 26 '15 at 22:48
  • @Stradigos, There are certainly situations where it would be wrong to use them (just as with everything else), but it is not an absolute rule. In any case, even if they are evil, the solution is not to expose member variables by making them public. – Mustafa Jan 26 '15 at 23:05
  • "Even if they are evil, the solution is not to expose member variables" How would access them then if it weren't for a getter/setter? If you wouldn't mind amending your answer, I'd enjoy seeing what you have in mind. – Stradigos Jan 26 '15 at 23:17
  • @Stradigos, I added a few suggestions for a safer design to my answer. They are a good start. I hope that helps. – Mustafa Jan 27 '15 at 16:34
0

Evil solution

If you don't mind spending sizeof(void*) extra bytes on your Shape object, you can add wrapper class (and wrapper object) into your Shape class:

class ShapeVerticies{
public:
    ShapeVerticies(Shape* shape)
        : shape(shape){
    }

    void operator=(const std::vector<vec3>& a){
        shape->geometry->verticies = a;
    }

    Shape* shape;
};
ShapeVerticies verticies;

You should initialize verticies with this in Shape constructor, of course.

Eviler solution

And you can reduce this space requirement to a single byte (probably single, that is a compiler choice). Anyway, I warn you not to do that.

class ShapeVerticiesEvil{
public:
    void operator=(const std::vector<vec3>& a){
        Shape* thisShape = reinterpret_cast<Shape*>(reinterpret_cast<unsigned char*>(this) - offsetof(Shape, verticies));
        thisShape->geometry->verticies = a;
    }
};
ShapeVerticiesEvil verticiesEvil;