• Advertisement
Sign in to follow this  

Getting rid of Get/Set functions

This topic is 4902 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

Still a bad example. In practice you'll never call Triangle::Draw() because it's too damn slow to do a function call for each triangle. You'll most likely call DrawTriangles(const vector<Triangle> &Triangles). If you want to ensure triangles don't get out of the screen you should ensure this at a higher abstraction level.

Share this post


Link to post
Share on other sites
Advertisement
Really cool discussion :)

I've seen classes like the following:


class CPoint:
{
float getX() const { return x; }
float getY() const { return y; }
float getZ() const { return z; }

void setX(float X) { x = X; }
void setY(float Y) { y = Y; }
void setZ(float Z) { z = Z; }

private;
float x,y,z;
}



I've never understood why people did it but I thought it was 'the rule to follow' so I must admit my code looks like this.. After reading this thread I'm really going to remove this stuff :)

But what do you guys think of something like:
Renderer->getDevice()->SomeFunction ();

There is no set function for my device so it shouldn't be public but code with lots of those render->getdevice statements is looking terrible. Is there a solution for this?

Share this post


Link to post
Share on other sites
Quote:
Original post by Scheermesje
But what do you guys think of something like:
Renderer->getDevice()->SomeFunction ();

There is no set function for my device so it shouldn't be public but code with lots of those render->getdevice statements is looking terrible. Is there a solution for this?

I use code like that too. Its a lot better than exposing the m_pDevice (or whatever you call it) member variable, since using a GetDevice() stops someone from setting the pointer to something else and breaking the code. Effectively, its read only.
I don't think it looks ugly at all, but thats just my opinion

Share this post


Link to post
Share on other sites
There was a discussion on a gamedev mailing list..

The discussion went about the following type of statement:

CFramework::Get().Log->Get().Write('');

It are not really set/get functions since it's a singleton but the problem is somewhat similiar.. Should you make the Log file a public member of your Framework or maybe make it global so you can remove all the set/get thingys.

And also if you would change something like this:


Render.getDevice()->SetTextureStageState(0, D3DTSS_ALPHAARG1, D3DTA_DIFFUSE);
Render.getDevice()->SetTextureStageState(0, D3DTSS_ALPHAOP,D3DTOP_SELECTARG1);

Render.getDevice()->SetRenderState(D3DRS_SRCBLEND, D3DBLEND_SRCALPHA);
Render.getDevice()->SetRenderState(D3DRS_DESTBLEND, D3DBLEND_INVSRCALPHA);

to

LPDIRECT3DDEVICE9 TempRender = Render.getDevice();
// Set the alpha stages
TempRender->SetTextureStageState(0, D3DTSS_ALPHAARG1, D3DTA_DIFFUSE);
TempRender->SetTextureStageState(0, D3DTSS_ALPHAOP,D3DTOP_SELECTARG1);

TempRender->SetRenderState(D3DRS_SRCBLEND, D3DBLEND_SRCALPHA);
TempRender->SetRenderState(D3DRS_DESTBLEND, D3DBLEND_INVSRCALPHA);



Something like this could clean up your code.. some other things where mentioned in that discussion..

Has anyone else read it? I can't find it anymore :(

Share this post


Link to post
Share on other sites
Quote:
Original post by Evil Steve
Quote:
Original post by Scheermesje
But what do you guys think of something like:
Renderer->getDevice()->SomeFunction ();

There is no set function for my device so it shouldn't be public but code with lots of those render->getdevice statements is looking terrible. Is there a solution for this?

I use code like that too. Its a lot better than exposing the m_pDevice (or whatever you call it) member variable, since using a GetDevice() stops someone from setting the pointer to something else and breaking the code. Effectively, its read only.
I don't think it looks ugly at all, but thats just my opinion


So the Renderer isn't adequate enough to do what you want without you accessing its implementation details. Sounds like a problem with your design.

Share this post


Link to post
Share on other sites
Sounds like you're right ;)

I know it's better to create functions that you can use to acces the API trough your own interface..

But it results in a lot of functions (10 set/get functions is not a lot if you're going to create your own renderer interface) so I'm still thinking about it ;)

Share this post


Link to post
Share on other sites
The GetDevice() call is wholly redundant. Why not just renderDevice->SomeFunction()?

After all, the Renderer class should know how to *render* things instead of providing a "GetDevice()" accessor which then knows how to render things.

Share this post


Link to post
Share on other sites
Quote:
Original post by CoffeeMug
Quote:
Original post by Magmai Kai Holmlor
In all areas of programming, get/set tuples are a code smell.

Well, what about a trivial example like this:


Doesn't matter; set/get tuple are only useful if they do something. If all they do is drop the object back, they're not useful (fail to abstract). If they actually do something, then they are poorly named.
Get/set is poor programming for essentially the same reason the use of the word "get" is poor grammar. You should be more explicit - get/set doesn't tell you anything.

For the first case, I hope it's clear to everyone that the triangle code above is absolutely pointless and makes the class *harder* to use and understand.

Your Get/SetWindowSize example is the second case. I'd call it Window::Resize, you're not just setting the size (cached for later use?), you're causing an avalanche of changes.

GetWindowSize isn't so objectionable, but the 'get' still doesn't tell you anything. Size is OK, but slightly ambiguous (size of the client/viewport area?), I think I'd use Window::Extents, though it wouldn't be hard to convince me to use Window::Size.

It might seem trivial, but code is names. The code still compiles if all the classes and variables are named x1, x2, x3, etc...

Share this post


Link to post
Share on other sites
highly interesting discussion.
just to clarify your point, Magmai Kai Holmlor -
Quote:

original post by Magmai Kai Holmlor
Doesn't matter; set/get tuple are only useful if they do something. If all they do is drop the object back, they're not useful (fail to abstract). If they actually do something, then they are poorly named.

so are you saying that the difference between these functions

// 1.
void SetX( int nX ) { m_x = nX; }
// 2.
void SetX( int nX ) { if( nX >= 0 && nX < some_predefined_variable ) { m_x = nX; } }
// 3.
void RedefineX( int nX ) { if( nX >= 0 && nX < some_predefined_variable ) { m_x = nX; } }



is that 1. is not useful because it doesn't do something, but 2. is actually useful because it does ? if so, this begs the question on how much functionality a function should have before it is classified above a petty "setter" or "getter".
edit : and apparently 3. is the best because of its more descriptive name.

Share this post


Link to post
Share on other sites
Quote:
Original post by antareus
[...]The problem with properties is that you essentially have to store two member function pointers for a little syntactic sugar.[...]
Are you sure that it wouldn't be inlined if coded properly? You could make the function pointers template parameters, in which case it should be done entirely compile time with no need to actually store the function pointers. You *would* have to have a 'parent' pointer of some kind, but maybe the compiler would be smart enough to make all the properties of a class share the same single pointer or somesuch if they stored a reference or something like that?

Share this post


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

class CPoint:
{
float getX() const { return x; }
float getY() const { return y; }
float getZ() const { return z; }

void setX(float X) { x = X; }
void setY(float Y) { y = Y; }
void setZ(float Z) { z = Z; }

private;
float x,y,z;
}



classic example of complete redundancy

1. In that example the getter/setters does not provide any abstraction what so ever.

3. You may at first believe this provides loose coupling among types/modules, if you think about it really it does not in any way for every member you have a corresponding get"insert member name" and set"insert member name" so clients still are indirectly tightly coupled to representation.

3. A point in 3-space is always representated by a tuple (3 scalar values), there is no variation in representation thats one reason to not make it private. There is just one thing thou a tuple could be representated by named variables or an array of 3 elements but there is good way to "Have Your Cake and Eat It Too" i'll demonstrate at the end.

4. A point in 3-space has no state invariant to maintain, what would it mean to have one besides? so thats another reason to not make it private.

This is just to restrictive for no good reason what so ever.

"Have Your Cake and Eat It Too"



#include <cstddef>

template < typename T >
class point3 {

typedef T point3<T>::* const p3[3]; //<--- pointer to data members
static const p3 v; //<- private, clients never know about it.

public:

typedef size_t size_type;

T x, y, z;

/* ... omitting code ... */

const T& operator[](size_type i) const {
return this->*v;
}

T& operator[](size_type i) {
return this->*v;
}

/* ... omitting code ... */
};

template < typename T >
const typename point3<T>::p3 point3<T>::v = { &point3<T>::x, &point3<T>::y, &point3<T>::z };



Before anyone trys to post anything about using anonymous unions read this thread completely first.

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement