Trouble accessing member class' function

[smoermeli]

#include <iostream>
#include <vector>
 
class Thing {
    private:
        int value;
        friend class Person;
    public:
        int getValue() { return value; }
        void setValue(const int val) { value=val; }
        Thing (const int val) { value=val; } // Constructor
};
 
class Person {
    private:
        std::vector<Thing> things; // All of Person's Things are stored in a vector
    public:
        void addThing(const int value) { things.push_back(Thing(value)); } // Adds Things to vector
        Thing getThing(const int i) { return things[i]; }
        int getThingValue(const int i) { return things[i].getValue(); } // Get and set value via Thing member function call
        void setThingValue(const int i, const int value) { things[i].setValue(value); }
        int getThingValueFriend(const int i) { return things[i].value; } // Get and set value via friendship from Thing
        void setThingValueFriend(const int i, const int value) { things[i].value = value; }
};
 
int main(int argc, char* argv[]) {
    Person* p = new Person();
    p->addThing(123);
    Thing temp = p->getThing(0);
    std::cout << temp.getValue() << std::endl; // #1: This works
    std::cout << p->getThingValue(0) << std::endl; // #2: As do these two
    std::cout << p->getThingValueFriend(0) << std::endl; // #3: ...
    std::cout << p->things[0].getValue(); // #4: But this doesn't
    delete p;
    return EXIT_SUCCESS;
}
 

I think the code is pretty straightforward; I've removed everything irrelevant, and compressed the whole thing into one chunk. The data structuring won't make much sense now that the program's just a skeleton, but bear with me.

While I realize the "orthodox" way of getting a Person's Thing's value would be #2 or #3, method #4 perplexes me. My compiler (MinGW/gcc 3.4.2) gives this error message:
16 `std::vector<Thing, std::allocator<Thing> > Person::things' is private
33 within this context

I can't figure out how to make expressions like p->things[0].getValue() work, nor why they won't work. I suspect it's because of the expression being evaluated in main's namespace or something, since declaring Person and Thing each other's friends doesn't seem to affect the error at all, but I'm not really sure.

Anyway, while I can (and will) use a better approach to accessing Thing's value, I'd appreciate it if someone could explain to me why method #4 doesn't work, and tell me the simple correction to be made that will leave me banging my head against the desk for not getting it. Thanks.

[afraze]

smoermeli,

std::vector<Thing> things;

Hi there! i think if you declare this code in class Person's public. Because i compile your code with no error.

p->things[0].getValue(); // also this works..

[smoermeli]

Argh, of course. All the time I just kept thinking that if I declared the vector public, it would expose the data fields of the objects contained in it - but now that I read your suggestion and thought it over, of course it doesn't expose them. Now I feel stupid.

Thanks. =)

[weaknessforcats]

Hi there! i think if you declare this code in class Person's public.

All data is private. Making this public exposes the vector to the world. Now anyone can put junk in the vector. AND if you ever need to change the vector into some other type of container, you will have to change all of the user code that referred to that vector. As a result code like this is called monolithic (one block of stone) because logic and variables are intertwined. Any major resdesign requires a scrap of the original code followed by a rewrite.

The reason #4 won't work is because there is no Person::getValue() method.

So write one:

int Person::getValue()
{
     return this->Thing::getValue() 
}
 

This is just a simple wrapper.

Making data public to get around an error is not wise. The compiler is trying to tell you that your design is faulty. Public data bypasses the fault.

[smoermeli]

I'm not sure how that wrapper should work, my compiler says: `Thing' is not a base of `Person'

But anyway, after a good night's sleep I figured out what was the mistake in my thinking. What I initially wanted to do was something like this:
Suppose I want Person to have another type of data, also contained in a vector, for example std::vector<OtherThing> otherthings, where OtherThing could be a subclass of Thing or some other class that has a similar getValue() function. Instead of writing separate functions in Person for every different type and accessing them like this:

p->getThingValue(index);
p->getOtherThingValue(index);

...and was wondering if I could somehow make them accessible like:

p->things[index].getValue();
p->otherThings[index].getValue();

What I didn't understand yesterday, but realize now, was that the problem wasn't about Thing or Person accessing things but main accessing it in order to reach Thing's getValue() function; please correct me if I got that wrong. I also understand now why declaring things public is a Bad Idea. Anyway, thanks for your help.

[afraze]

May be public declaration is not good ..
But it is important that;

1) for what our code will be used?

2) if it is not a big project and just study, we can expose our data to everyone dont be afraid!

[weaknessforcats]

my compiler says: `Thing' is not a base of `Person'

Yeah, well so much for compile by eyeball.

This one works:

int Person::getValue(int arg)
{
     return this->things[arg].getValue();
}
 

but realize now, was that the problem wasn't about Thing or Person accessing things but main accessing it in order to reach Thing's getValue() function; please correct me if I got that wrong.

You have the correct view. The essence of encapsulation is to measure how much code breaks if you change a line of code. Breaking 1000 lines of code rather than 5000 lines of code means better encapsulation. By using private access specifiers, you limit your damage to the member functions.