Editing member variable of a shared object

[Jimbot]

Hello,

I am having more problems editing member variables inside vectors of objects (similar to a previous question of mine: http://www.cplusplus.com/forum/beginner/41015/)

This time round I have 3 classes:

Object - each object has an owner
Owner - an owner has a name and a value
Collection - has a vector of Objects

I want to create a single Collection and populate its vector with 5 Objects each with the same Owner.

I then print out the Owner and Value of each Object but edit the value in the middle.

Seeing as they all have a shared single Owner I would expect the change to the Value member to take effect across all Objects but it does not - why is that?

Here is my code:

#include <iostream>
#include <vector>

using namespace std;

class Owner
{
	public:
		Owner();
		Owner(string NewName, int NewValue);
		string GetName();
		int GetValue();
		void SetName(string NewName);
		void SetValue(int NewValue);

	private:
		string Name;
		int Value;
};

Owner::Owner()
{
}

Owner::Owner(string NewName, int NewValue)
{
	Name = NewName;
	Value = NewValue;
}

string Owner::GetName()
{
	return Name;
}

int Owner::GetValue()
{
	return Value;
}

void Owner::SetValue(int NewValue)
{
	Value = NewValue;
}

class Object
{
	public:
		Object();
		Object(Owner NewOwner);
		Owner& GetOwner();
	private:
		Owner ObjectOwner;
};

Object::Object()
{
}
	
Object::Object(Owner NewOwner)
{
	ObjectOwner = NewOwner;
}

Owner& Object::GetOwner()
{
	return ObjectOwner;
}

class Collection
{
	public:
		void AddObject(Object NewObject);
		Object& GetObject(int ObjectIndex);

	private:
		vector<Object> AllObjects;
};

void Collection::AddObject(Object NewObject)
{
	AllObjects.push_back(NewObject);
}

Object& Collection::GetObject(int ObjectIndex)
{
	return AllObjects[ObjectIndex];
}

int main()
{
	/* Create Owner with a default value*/
	Owner NewOwner("Jimbot", 5);

	/* Start new collection */
	Collection NewCollection;

	/* Populate the Collection with 5 Objects each with the same owner */
	for(int i = 0; i < 5; i++)
	{
		NewCollection.AddObject(Object(NewOwner));
	}
	
	/* Print out the name of the owner and value of each object */
	for(int i = 0; i < 5; i++)
	{
		Object& CurrentObject = NewCollection.GetObject(i);
		Owner& CurrentOwner = CurrentObject.GetOwner();
		cout <<	CurrentOwner.GetName() << endl;
		/* Edit the value of one of the objects owner */
		if(i == 3)
		{
			CurrentOwner.SetValue(10);
		}
		cout <<	CurrentOwner.GetValue() << endl;
	}

	return 0;
}

I would expect an output of:

Jimbot
5
Jimbot
5
Jimbot
5
Jimbot
10
Jimbot
10

But what I get is:

Jimbot
5
Jimbot
5
Jimbot
5
Jimbot
10
Jimbot
5

What am I doing wrong?

Thanks for any guidance,

Jimbot

[ne555]

class A{
  B b;
};

Every A has a different instance of B (they may have the same values, but they are different)

You need to maintain references instead, so every A 'points' to a B (and they could be shared)

class A{
  B &b; //once set it cannot be changed (and always need one)
};
//or
class A{
  B *b;  //could be NULL, and can change at runtime
};

[Jimbot]

Hi ne555,

Thanks for your reply and help.

So although I am returning references with my class accessor functions they are to member variables which are unique to each class instance?

The not being able to change once set is not much of a problem for my application but I guess I can't use a reference as a member variable as it needs initialising first (and then once initiliased can't be changed?)

I think your second suggestion of a pointer would work best as I want to be able to set it at runtime (and then leave it set)

Thanks for any further guidance,

Jimbot

[dangrr888]

Yes, you are adding a separate yet identical object to the elements of the vector<Object> of your collection object. Hence, your if statement in main() only reassigns data members of NewCollection.AllObjects[3].

Several other points:

-I would recommend using member initialization lists to initialize your data members, otherwise you default initialize them and then reinitialize them again in the constructors, this is inefficient.

- You should make your accessor functions constant member functions, this prevents you or others accidentally performing bitwise non-const operations in such methods.

- You should be passing object arguments by reference-to-const, otherwise you are passing by value and copying them needlessly. This is inefficient and could become problematic as you expand the implementation of these classes.

-Rather than using composition, I would have each object of the Object and Collection container classes possess a tr1::shared_ptr<Owner> or tr1::shared_ptr<vector<Object> > rather than an Owner or vector<Object> respectively. This prevents the Object or Collection classes becoming too large.

-Also, if you use tr1::shared_ptrs you can have several Objects share the same Owner object, or several elements of the vector pointed to by a Collection share the same Object that you have declared earlier. This is behaviour you are looking for, I believe.

-Lastly, you should really return references to private data members, which goes against the encapsulation ethos of your classes, i.e., making the Object/Owner data members private.

Hope this helps.

[Jimbot]

dangrr888,

Thank you very much for your help.

I would recommend using member initialization lists to initialize your data members, otherwise you default initialize them and then reinitialize them again in the constructors, this is inefficient.

Thank you for this - just looked them up and they will be useful in my program (I was seeing constructors being called after initialisation)

- You should make your accessor functions constant member functions, this prevents you or others accidentally performing bitwise non-const operations in such methods.

- You should be passing object arguments by reference-to-const, otherwise you are passing by value and copying them needlessly. This is inefficient and could become problematic as you expand the implementation of these classes.

I am actually doing this in my 'real' code but left it out of my forum example for the sake of succinctness.

Rather than using composition, I would have each object of the Object and Collection container classes possess a tr1::shared_ptr<Owner> or tr1::shared_ptr<vector<Object> > rather than an Owner or vector<Object> respectively. This prevents the Object or Collection classes becoming too large.

-Also, if you use tr1::shared_ptrs you can have several Objects share the same Owner object, or several elements of the vector pointed to by a Collection share the same Object that you have declared earlier. This is behaviour you are looking for, I believe.

I will read up on this although I feel it may not be necessary for my purposes as my class structure will not be particularly large.

Lastly, you should really return references to private data members, which goes against the encapsulation ethos of your classes, i.e., making the Object/Owner data members private.

I think you mean "you should not really return..." (??) - I can see how this could go against the encapsulation ethos but I am still unclear as to how private members can be updated (especially through layers of classes/inheritance) I brought this up in this post: http://www.cplusplus.com/forum/general/49783/

The BlackJack game which I mention in that post is what this code is for - I have a table class which contains a vector of 'boxes' (seats at the blackjack table). Each box has an owner (a player object). For most of the time each box has a separate owner, but if a player chooses to split their cards then two (or more) boxes will temporarily (until the end of the round) share an owner - it is this functionality which I am working on (struggling with) at the moment.

Thanks again for your help and any further guidance,

Jimbot

[dangrr888]

Hi Jimbot,

great, glad to help!

I will read up on this although I feel it may not be necessary for my purposes as my class structure will not be particularly large.

its good practice to use resource managing objects such as smart pointers (e.g., auto_ptr and tr1::shared_ptr), for many reasons (see Effective C++ by Myers, Chapter 3). Here, you don't know how you'll want to expand your code in the future.

think you mean "you should not really return..." (??) - I can see how this could go against the encapsulation ethos but I am still unclear as to how private members can be updated (especially through layers of classes/inheritance) I brought this up in this post: http://www.cplusplus.com/forum/general/49783/

Yeah, I understand what you mean. The resolution to this is that you don't provide direct access to your private members via references, but you provide write access through member functions alone. This is what you are in your Setting member functions (e.g., Owner::SetValue) where you allow write access but don't return references to private data members.

The reason you should do things this way is that you may wish data members to be restricted to certain values that your member functions check for via a set of specified conditions. If you allow clients direct access to private members then you have no way of restricting what they do to them, and this could cause problems.

Hope that helps. I would definitely recommend Effective C++ by Myers, it has all these little rules spelled out nicely.

[dangrr888]

Oh, one more thing I forgot to mention.

In the following:

for(int i = 0; i < 5; i++)
	{
		NewCollection.AddObject(Object(NewOwner));
	}

you don't need to explicitly declare NewOwner to be an Object as an argument to AddObject. You have already declared NewOwner an object.

All your doing is calling the synthesised Copy Constructor for Object to create a clone of NewOwner and add it to NewCollection. Since you are passing by value in Collection::AddObject you are now copying NewOwner twice! As I said above, you should be passing objects by reference-to-const.

[Jimbot]

Thanks again dangrr888!

I'll check out that Myers book - sounds good.

you don't need to explicitly declare NewOwner to be an Object as an argument to AddObject. You have already declared NewOwner an object.

Good spot with this - I will correct this (I've done it in plenty of other places!)