Sign in to follow this  
shotgunnutter

Read only public members of class.

Recommended Posts

I wonder if this is a good alternative to using get/set methods. I'm currently writing a class to do explosions, which has quite a lot of variables which need to be read by other classes within different subsystems. Suppose I was to do this:
class roFloat
{
private:
  float value;
  bool locked;
public:
  roFloat(){unlock()};
  roFloat(float V){value=V; lock();};
  void lock();
  void unlock();
};

//overloaded operators for read access: always work
roFloat operator== ...
roFloat operator+ ...
roFloat operator- ...


//overloaded operators for write access: only work if instance is unlocked.
roFloat operator= ----throw assertion to warn programmer
roFloat operator+= ----throw assertion to warn programmer
...etc.



Is there a commonly accepted alternative to doing this? I've certainly never seen anything like it. [edit] Oh, and don't worry, I'm wearing my bullet proof shoes. [lol] [Edited by - shotgunnutter on December 30, 2007 3:48:33 PM]

Share this post


Link to post
Share on other sites
Quote:
Original post by hydroo
*** Source Snippet Removed ***


That implementation still adds an extra member variable. I'm looking to avoid something like this:


class explosion
{
private:
float age
float force_distribution_matrix[3][3][3];
float force;
float radius;
float force_left;
float temperature;
...plus some physics engine specific objects

public:
float get_age();
float * get_force_distribution_matrix();
float get_force();
float get_radius;
float get_energy_left();
float get_temperature();

};




instead having something like this:


class explosion
{
private:

public:
roMember<float>age;
roMember<float>force;
roMember<float> * force_distribution_matrix;
...
};




so that the first time a public member is set, it then locks. Take the force distribution matrix, for example, in the constructor:


force_distro_matrix[0][0][0]=some_value;
force_distro_matrix[1][0][0]=some_value;
force_distro_matrix[2][0][0]=some_value;
force_distro_matrix[3][0][0]=some_value;




then, the AI, the Collision system, the game logic, and the renderer are all able to access the force distrobutiuon matrix by direct naming. Anybody subsequently abusing the system, and trying to edit the public members, would get a failed assertion.

The question, basically, is would this implementation be a bad idea? I don't have any professional c++ experience to apply here.

[Edited by - shotgunnutter on December 30, 2007 4:04:34 PM]

Share this post


Link to post
Share on other sites
You could also make those functions friend of your class.

Or make them public? I mean, if you look for a way to read and write the variables without getters/setters, they'll become public anyway, or else you can make only that what's allowed to read/write friend.

Or make something like this?


class Foo
{
private:
int bar;

public:
const int& getBar() const { return bar; }
};



Edit: after the other reply from shotgunnutter in the middle it seems that I answered something wrong.

Did you just answer your own question with the part under "instead having something like this"? Give your templated ro's the necessary operator=, operator float(), etc..., and you can get the behaviour you want?

Share this post


Link to post
Share on other sites
Quote:
Original post by Lode
You could also make those functions friend of your class.

Or make them public? I mean, if you look for a way to read and write the variables without getters/setters, they'll become public anyway, or else you can make only that what's allowed to read/write friend.

Or make something like this?

*** Source Snippet Removed ***


hmm. My concern here is reducing the conceptual overhead. the longer I think about simulating an explosion, the more private members I come up with. Your method in fact has three members for each method. Since the current class, which I will use for testing the principles of the force distribution matrix, has 13 member variables, not to mention the 21 within in the distribution matrix, that means 39 members!

Share this post


Link to post
Share on other sites
Quote:
Original post by Lode

...
Edit: after the other reply from shotgunnutter in the middle it seems that I answered something wrong.

Did you just answer your own question with the part under "instead having something like this"? Give your templated ro's the necessary operator=, operator float(), etc..., and you can get the behaviour you want?


no, I'm considering that, I was asking *if* that would be a good alternative to having getters for each and keeping them private. Just to clarify. This could be another example of shooting oneself in the lower leg.

Share this post


Link to post
Share on other sites
Well, personally I like the approach with the templates, it avoids a lot of code duplication, but maybe that's just me. I think it's maintainable (putting all the logic concerning the read/write stuff in 1 place in the template class), readable, and avoids code duplication so... why not? :)

What you could do extra is put all the roMember<float>'s in a struct ExplosionData and then in your class make 1 getter to get the struct. That is, if you want to avoid having public data members in classes (in a struct that serves only to collect data it's more commonly accepted to have public data).

Share this post


Link to post
Share on other sites
Quote:
Original post by Lode
Well, personally I like the approach with the templates, it avoids a lot of code duplication, but maybe that's just me. I think it's maintainable (putting all the logic concerning the read/write stuff in 1 place in the template class), readable, and avoids code duplication so... why not? :)

What you could do extra is put all the roMember<float>'s in a struct ExplosionData and then in your class make 1 getter to get the struct. That is, if you want to avoid having public data members in classes (in a struct that serves only to collect data it's more commonly accepted to have public data).


OK. Due to this discussion I have decided to try it by the method in my second post. I'll finish the whole thing in a couple of hours and post the implementation back.

Thanks for the advice.

Share this post


Link to post
Share on other sites
I would prefer to implement such classes by (for example) moving explosion's behaviour into the explosion class such that clients do not need to get and set variables but rather ask explosion instances to do things:
class explosion
{
private:
/* stuff */

public:
void update(std::clock_t time_delta);
void affect(thing & thing) const;

};

Σnigma

Share this post


Link to post
Share on other sites
Quote:
Original post by Enigma
I would prefer to implement such classes by (for example) moving explosion's behaviour into the explosion class such that clients do not need to get and set variables but rather ask explosion instances to do things


Quoted for extreme emphasis.

Share this post


Link to post
Share on other sites
Quote:
Original post by Enigma
I would prefer to implement such classes by (for example) moving explosion's behaviour into the explosion class such that clients do not need to get and set variables but rather ask explosion instances to do things:
*** Source Snippet Removed ***
Σnigma


I considered that as well. But, since the explosion class is visible to the renderer, the AI, and the physics engine (among others) that would result in a lot of functionality embedded in the explosion class.

The alternative, of course, is to have more than one kind of explosion, one for each system that recognises an "explosion" as an event, object to avoid, something to draw, etc.

Share this post


Link to post
Share on other sites
A generic "read-only" template could look like this:


template <typename T, typename FriendType>
class ReadOnly
{
public:
friend FriendType;

operator const T() const // adds elegance though also a source for errors!
{
return _value;
}

const T get() const // or const T operator()() const
{
return _value;
}

private:

ReadOnly() : _value() {}

ReadOnly(const ReadOnly & copy_) : _value(copy_.get()) {}

ReadOnly(const T & assign_) : _value(assign_) {}

ReadOnly& operator =(T assignCopy_);
//....

T _value;
};

//So a read-only member would look like this:

class SomeClass
{
public:
ReadOnly<int, SomeClass> readOnlyInt;
};



Share this post


Link to post
Share on other sites
Quote:
Original post by kiome
A generic "read-only" template could look like this:

*** Source Snippet Removed ***


Thanks! I was just halfway through researching that on cplusplus.com, and tearing my hair out while I was at it. Except, it doesn't compile on gcc:

"/Users/shotgunnutter/Projects/ode_collision/current_copy/readonly.h:14: error: a class-key must be used when declaring a friend
/Users/shotgunnutter/Projects/ode_collision/current_copy/readonly.h:14: error: friend declaration does not name a class or function"


template <typename T, typename FriendType>
class ReadOnly
{
public:
friend FriendType;
^^^^errors are on this line^^^^^^

operator const T() const // adds elegance though also a source for errors!
{
return _value;
}


Share this post


Link to post
Share on other sites
Quote:
Original post by shotgunnutter

Thanks! I was just halfway through researching that on cplusplus.com, and tearing my hair out while I was at it. Except, it doesn't compile on gcc:

"/Users/shotgunnutter/Projects/ode_collision/current_copy/readonly.h:14: error: a class-key must be used when declaring a friend
/Users/shotgunnutter/Projects/ode_collision/current_copy/readonly.h:14: error: friend declaration does not name a class or function"

*** Source Snippet Removed ***


Hi, the syntax for friend is:

class A
{
int i;
friend class B;
friend void f();
};

class B
{
public:
int j()
{
A a;
a.i = 5;
}
};

void f()
{
A a;
a.i = 5;
};

Share this post


Link to post
Share on other sites
Well, I've come to the conclusion that explosions are a bit of a kludge any way you look at it. I think I'll just bodge something up as a prototype, and think about a final version later, with the benefit of hindsight.

Thanks all for helping me on my thought process.

Share this post


Link to post
Share on other sites
Quote:
Original post by kiome
Try writing friend typename FrientType; instead.


Strange. If i do this I get told
"
/Users/shotgunnutter/Projects/ode_collision/current_copy/readonly.h:14: error: expected nested-name-specifier before 'FriendType'
/Users/shotgunnutter/Projects/ode_collision/current_copy/readonly.h:14: error: 'FriendType' is neither function nor member function; cannot be declared friend
"

But if I delete the friend declaration it compiles just fine, including the following (n.b. I had to make the ReadOnly::constructors public)


class SomeClass
{
public:
ReadOnly<int, SomeClass> readOnlyInt;
SomeClass()
{
readOnlyInt = 4;
};

};





[edit: wait, no it doesnt. I made the operator= public along with the constructors by mistake. When rectified, the lack of a friend identifier comes back to haunt me:

"
Building target “OpenGL Test Project” of project “game_engine” with configuration “Development” — (8 errors, 1 warning)
cd /Users/shotgunnutter/Projects/ode_collision/current_copy
/usr/bin/gcc-4.0 -x c++ -arch i386 -pipe -Wno-trigraphs -fpascal-strings -fasm-blocks -g -O0 -fmessage-length=0 -mfix-and-continue -fvisibility-inlines-hidden -I/Users/shotgunnutter/Projects/ode_collision/current_copy/build/game_engine.build/Development/OpenGL\ Test\ Project.build/MD2\ Model.hmap -Wmost -Wno-four-char-constants -Wno-unknown-pragmas -O0 -F/Users/shotgunnutter/Projects/ode_collision/current_copy/build/Development -I/Users/shotgunnutter/Projects/ode_collision/current_copy/build/Development/include -I/Users/shotgunnutter/Projects/ode_collision/current_copy/build/game_engine.build/Development/OpenGL\ Test\ Project.build/DerivedSources -include /Library/Caches/com.apple.Xcode.505/SharedPrecompiledHeaders/Prefix-dcdcbvotmmwmnhajaudltmkzrsfz/Prefix.pch -c /Users/shotgunnutter/Projects/ode_collision/current_copy/Source/Main.cpp -o /Users/shotgunnutter/Projects/ode_collision/current_copy/build/game_engine.build/Development/OpenGL\ Test\ Project.build/Objects-normal/i386/Main.o
/Users/shotgunnutter/Projects/ode_collision/current_copy/readonly.h:14: error: expected unqualified-id before '<' token
/Users/shotgunnutter/Projects/ode_collision/current_copy/readonly.h: In constructor 'SomeClass::SomeClass()':
/Users/shotgunnutter/Projects/ode_collision/current_copy/readonly.h:32: error: 'ReadOnly<T, FriendType>& ReadOnly<T, FriendType>::operator=(T) [with T = int, FriendType = SomeClass]' is private
/Users/shotgunnutter/Projects/ode_collision/current_copy/readonly.h:46: error: within this context
/Users/shotgunnutter/Projects/ode_collision/current_copy/Source/Main.cpp: In function 'int SDL_main(int, char**)':
/Users/shotgunnutter/Projects/ode_collision/current_copy/readonly.h:32: error: 'ReadOnly<T, FriendType>& ReadOnly<T, FriendType>::operator=(T) [with T = int, FriendType = SomeClass]' is private
/Users/shotgunnutter/Projects/ode_collision/current_copy/Source/Main.cpp:44: error: within this context
/Users/shotgunnutter/Projects/ode_collision/current_copy/Source/Main.cpp:43: warning: unused variable 'f'
/Users/shotgunnutter/Projects/ode_collision/current_copy/readonly.h:14: error: expected unqualified-id before '<' token
/Users/shotgunnutter/Projects/ode_collision/current_copy/readonly.h:32: error: 'ReadOnly<T, FriendType>& ReadOnly<T, FriendType>::operator=(T) [with T = int, FriendType = SomeClass]' is private
/Users/shotgunnutter/Projects/ode_collision/current_copy/readonly.h:46: error: within this context
/Users/shotgunnutter/Projects/ode_collision/current_copy/readonly.h:32: error: 'ReadOnly<T, FriendType>& ReadOnly<T, FriendType>::operator=(T) [with T = int, FriendType = SomeClass]' is private
/Users/shotgunnutter/Projects/ode_collision/current_copy/Source/Main.cpp:44: error: within this context
/Users/shotgunnutter/Projects/ode_collision/current_copy/Source/Main.cpp:43: warning: unused variable 'f'
cd /Users/shotgunnutter/Projects/ode_collision/current_copy
/usr/bin/gcc-4.0 -x c++ -arch i386 -pipe -Wno-trigraphs -fpascal-strings -fasm-blocks -g -O0 -fmessage-length=0 -mfix-and-continue -fvisibility-inlines-hidden -I/Users/shotgunnutter/Projects/ode_collision/current_copy/build/game_engine.build/Development/OpenGL\ Test\ Project.build/MD2\ Model.hmap -Wmost -Wno-four-char-constants -Wno-unknown-pragmas -O0 -F/Users/shotgunnutter/Projects/ode_collision/current_copy/build/Development -I/Users/shotgunnutter/Projects/ode_collision/current_copy/build/Development/include -I/Users/shotgunnutter/Projects/ode_collision/current_copy/build/game_engine.build/Development/OpenGL\ Test\ Project.build/DerivedSources -include /Library/Caches/com.apple.Xcode.505/SharedPrecompiledHeaders/Prefix-dcdcbvotmmwmnhajaudltmkzrsfz/Prefix.pch -c /Users/shotgunnutter/Projects/ode_collision/current_copy/readonly.cpp -o /Users/shotgunnutter/Projects/ode_collision/current_copy/build/game_engine.build/Development/OpenGL\ Test\ Project.build/Objects-normal/i386/readonly.o
/Users/shotgunnutter/Projects/ode_collision/current_copy/readonly.h:14: error: expected unqualified-id before '<' token
/Users/shotgunnutter/Projects/ode_collision/current_copy/readonly.h: In constructor 'SomeClass::SomeClass()':
/Users/shotgunnutter/Projects/ode_collision/current_copy/readonly.h:32: error: 'ReadOnly<T, FriendType>& ReadOnly<T, FriendType>::operator=(T) [with T = int, FriendType = SomeClass]' is private
/Users/shotgunnutter/Projects/ode_collision/current_copy/readonly.h:46: error: within this context
/Users/shotgunnutter/Projects/ode_collision/current_copy/readonly.h:14: error: expected unqualified-id before '<' token
/Users/shotgunnutter/Projects/ode_collision/current_copy/readonly.h:32: error: 'ReadOnly<T, FriendType>& ReadOnly<T, FriendType>::operator=(T) [with T = int, FriendType = SomeClass]' is private
/Users/shotgunnutter/Projects/ode_collision/current_copy/readonly.h:46: error: within this context
Build failed (8 errors, 1 warning)
"


[Edited by - shotgunnutter on December 30, 2007 6:32:32 PM]

Share this post


Link to post
Share on other sites
Removing the friend declaration would render _value unreachable for the class. My example compiles fine on VC, I am not that familiar with GCC, might try to get it to work once I find some free time.

Share this post


Link to post
Share on other sites
Quote:

A commonly accepted alternative to doing this?



Not an equivalent substitute mind you, but a commonly accepted alternative?:
This might sound a bit naive, but couldn't you use the const keyword in there
somewhere for a more typical solution?

It may not bulletproof your feet, but I am sure it could be used to show you that you are aiming the gun at your feet,... before you decide to (or not to..) pull the trigger (you know...one of the sort of things const can be used for) you could also save some of your bullets.

Share this post


Link to post
Share on other sites
I think gnu c++ may be lacking that functionality. I'm sure there is a way around it, however I'm going with Lode's idea of having the variables stored in a struct and having a single const getter to access the struct. It ticks all the boxes; I can have access to all the (many) variables required for the explosion, it allows the explosion class to modify its own behaviour, and it prevents other classes from corrupting its properties.

Share this post


Link to post
Share on other sites
Quote:

Original post by shotgunnutter
Thanks! I was just halfway through researching that on cplusplus.com, and tearing my hair out while I was at it. Except, it doesn't compile on gcc:

"/Users/shotgunnutter/Projects/ode_collision/current_copy/readonly.h:14: error: a class-key must be used when declaring a friend
/Users/shotgunnutter/Projects/ode_collision/current_copy/readonly.h:14: error: friend declaration does not name a class or function"


Try it with this addition:


template <typename T, typename FriendType>
class ReadOnly
{
public:
class FriendType; // <-- Added
friend FriendType;

operator const T() const // adds elegance though also a source for errors!
{
return _value;
}

const T get() const // or const T operator()() const
{
return _value;
}

private:

ReadOnly() : _value() {}

ReadOnly(const ReadOnly & copy_) : _value(copy_.get()) {}

ReadOnly(const T & assign_) : _value(assign_) {}

ReadOnly& operator =(T assignCopy_);
//....

T _value;
};

//So a read-only member would look like this:

class SomeClass
{
public:
ReadOnly<int, SomeClass> readOnlyInt;
};

Share this post


Link to post
Share on other sites
Quote:

Original post by shotgunnutter
Quote:
Original post by Enigma
I would prefer to implement such classes by (for example) moving explosion's behaviour into the explosion class such that clients do not need to get and set variables but rather ask explosion instances to do things:
*** Source Snippet Removed ***

I considered that as well. But, since the explosion class is visible to the renderer, the AI, and the physics engine (among others) that would result in a lot of functionality embedded in the explosion class.

The alternative, of course, is to have more than one kind of explosion, one for each system that recognises an "explosion" as an event, object to avoid, something to draw, etc.


Just out of curiosity - why don't you want to use this approach?

Share this post


Link to post
Share on other sites
[/quote]Just out of curiosity - why don't you want to use this approach?[/quote]
Consider the following:

player.setPosition(player.getPosition() + toMove);
and
player.move(toMove);

When you want to change something (say add collision checks) you are forced to do it wherever you have the set/get. code abstraction at its finest. In a perfect world the get() functions are meant for acquire the current state of the object, and set() for resetting the state(not modifying it)
Besides, I think it is more readable :)

Back on the topic...
This is a rather nice class that I whipped up just recently, it doesnt handle most operators(++, +=, +) , but then again those are not (really) needed(a += b == a =a+b). Someone might want to add those (and are free to do so).

#pragma warning(push)
#pragma warning(disable:4100)// aType is unreferenced, let's ignore warning
template<typename Type>
class ReturnTrue {
public:
bool operator()(const Type& aType) {
return true;
}
};
#pragma warning(pop)

template<typename Type, Type aConstValue>
class Above {
public:
bool operator()(const Type& aType) {
return aType > aConstValue;
}
};

template<typename Type, class Validator=ReturnTrue<Type> >
class Attribute {
public:
Attribute() : mIsLocked(false) {
}
explicit Attribute(const Type& aNewValue) : mIsLocked(false), mValue(aNewValue) {
AssertValid(aNewValue);
}
const Attribute& operator=(const Type& aNewValue) {
AssertValid(aNewValue);
AssertLocked();
mValue = aNewValue;
return *this;
}

operator const Type&() const {
AssertValid(mValue);
return mValue;
}

// constify this, prevent changes
void lock() {
mIsLocked = true;
}
void stream(std::istream& aStream) {
AssertLocked();
aStream >> mValue;
AssertValid(mValue);
}
void stream(std::ostream& aStream) const {
AssertValid(mValue);
aStream << mValue;
}
private:
static void AssertValid(const Type& aValue) {
assert( Validator()(aValue) );
}
void AssertLocked() {
assert(!mIsLocked);
}
Type mValue;
bool mIsLocked;
};
template<typename Type, class Validator>
std::ostream& operator<<(std::ostream& aStream, const Attribute<Type, Validator>& aAttribute) {
aAttribute.stream(aStream);
return aStream;
}
template<typename Type, class Validator>
std::istream& operator>>(std::istream& aStream, Attribute<Type, Validator>& aAttribute) {
aAttribute.stream(aStream);
return aStream;
}



Test case:
class Foo {
public:
Foo() : pInt(3), pFloat(0) {}
Attribute<int, Above<int, 0> > pInt;
Attribute<float> pFloat;
Attribute<std::string> pString;
};

void main() {
Foo bar;
bar.pString = "Hello world";
bar.pFloat = bar.pInt + 39.0f;
// bar.pInt = -5;
// assert() since pInt is going to be lower than 0

std::cout << bar.pString << std::endl
<< "The meaning of life is " << bar.pFloat << std::endl;
std::cin.get();
}

Share this post


Link to post
Share on other sites
Quote:

Original post by sirGustav
Quote:
Just out of curiosity - why don't you want to use this approach?

Consider the following:

player.setPosition(player.getPosition() + toMove);
and
player.move(toMove);

When you want to change something (say add collision checks) you are forced to do it wherever you have the set/get. code abstraction at its finest. In a perfect world the get() functions are meant for acquire the current state of the object, and set() for resetting the state(not modifying it)
Besides, I think it is more readable :)


It sounds like you also prefer to use the approach Enigma suggested, so I don't really understand what you're trying to say...

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this