Allegro.cc - Online Community

Allegro.cc Forums » Programming Questions » operator overloading question

Credits go to Karadoc ~~, Stas B., and Tobias Dammers for helping out!
This thread is locked; no one can reply to it. rss feed Print
 1   2 
operator overloading question
knudel
Member #13,235
August 2011

Hi,

I tried to write a keyboard-class to handle all keybaordfunctions easily with one object from this class. I'm using an int-array where I'm saving the state of each key. The plan was to get access to this array by overloading the operator (), so that I can handle it like this:

In Keyboard.h:

  //(...)
  public:
    int operator()(int);
  //(...)

In Keyboard.cpp:

 //(...)
 int Keyboard::operator()(int k){
   return button[k];
 }
 //(...)

And finally I want to use it like this:

int main(int argc, const char *argv[]){
   //(...)
   Keyboard key
   std::cout << key(ALLEGRO_KEY_UP);   //returns the value of the up-key and works
   Keyboard* key2 = new Keyboard();
   std::cout << key2(ALLEGRO_KEY_UP);  //doesn't work
}

The second way returns the error "error: 'key' cannot be used as a function.
Why am I not able to use the overloaded Operator with an pointer to an object?

thanks for all answers :)

Karadoc ~~
Member #2,749
September 2002
avatar

Because the pointer doesn't have the operator. Only the class itself has the operator. You'll have to deference it, I suppose.
(*key2)(ALLEGRO_KEY_UP)

-----------

Stas B.
Member #9,615
March 2008

That's a horrible idea. It's immoral. Please don't go crazy overloading operators. :(

jmasterx
Member #11,410
October 2009

key->button(ALLEGRO_KEY_UP);

kazzmir
Member #1,786
December 2001
avatar

Why not overload operator [] instead?

jmasterx
Member #11,410
October 2009

In all honesty, I don't think a keyboard key should also be able to translate into a game button in the first place.

knudel
Member #13,235
August 2011

thanks for all answers :)
Would kazzmir's solution - using the operator [] instead of () - be ok, if I use it like:

 Keyboard Key;
Key[ALLEGRO_KEY_A]

or shall I write a getButton-function?

Tobias Dammers
Member #2,604
August 2002
avatar

It's conceptually wrong. By overloading an operator, you make a promise that the operator will behave as it suggests. The () operator is supposed to work like a function call, which means that if your object provides it, it should be conceptually similar to a function. However, in no universe is a keyboard like a function in any way, and 'calling the keyboard' doesn't make sense.

The index operator [], which has been mentioned as an alternative, is somewhat better, but still wrong. A keyboard is not a collection of key states, it is a device that HAS a collection of key states (or it can PROVIDE one).

A full-blown "OOP-correct" solution would be to have the keyboard return an object that can then be used to query individual keys - naively, this object could be a simple array or vector, but for efficiency, you'd probably want a specialized class that looks up individual key states as needed ("lazy evaluation"). In practice, however, the best solution (in terms of KISS and other software robustness principles) is to just give the Keyboard class a getKeyState(int scancode) const method. The clarity and readability you win is well worth the bit of extra typing (more so if you use an IDE with auto-completion).

Just consider hypothetical usage of your class:

// compare this:
void foobar(const Keyboard& kbd) {
    bool a = kbd(27);
}
// against this:
void foobar(const Keyboard& kbd) {
    bool a = kbd.getKeyStatus(27);
}

The first example says "call the keyboard object with the number 27, and see if the result is true", while the second one says "from the keyboard object, get key status 27 and see if that is true". A reader can make wild guesses as to what "calling the keyboard" is supposed to mean, but "getting a key status" is a fairly obvious thing to do, and even though both the parameter and the return value are 'magic constants', an educated guess would inevitably lead to the conclusion that the argument must be a scancode and the return value probably means 'true' for 'pressed' and 'false' for 'released'. Using a symbolic constant for the scancode (e.g. SCANCODE_ESC), naming the method 'isKeyPressed' instead of 'getKeyStatus', and using a descriptive name for the variable would increase readability even more:

void foobar(const Keyboard& kbd) {
    bool escapePressed = kbd.isKeyPressed(Keyboard::SCANCODE_ESC);
}

With code like this, the reader doesn't have to look up anything at all, because the code documents itself.

---
Me make music: Triofobie
---
"We need Tobias and his awesome trombone, too." - Johan Halmén

Stas B.
Member #9,615
March 2008

A full-blown "OOP-correct" solution would be to have the keyboard return an object that can then be used to query individual keys - naively, this object could be a simple array or vector, but for efficiency, you'd probably want a specialized class that looks up individual key states as needed ("lazy evaluation").

Err... A full-blown "OOP-correct" solution would not expose any implementation details. There's absolutely no need for anyone to know that you use an array-like object to store your key states. In fact, returning references to objects belonging to an instance of your class is generally a horrible idea since you virtually can't protect them from being modified in unintended ways.
Way to go teaching newbies "OOP-correct" solutions. :-/

knudel
Member #13,235
August 2011

Thanks a lot :)
Now overloaded operators are much more plausible to me and I'll use the good code without them ;D

Slartibartfast
Member #8,789
June 2007
avatar

Stas B. said:

In fact, returning references to objects belonging to an instance of your class is generally a horrible idea since you virtually can't protect them from being modified in unintended ways.

Then return a reference to a const?

Stas B.
Member #9,615
March 2008

O rly?

#SelectExpand
1class B { 2 public: 3 char* ptr; 4}; 5 6class A { 7 B b; 8 public: 9 10 B const& getB() { return b; } 11}; 12 13int main() 14{ 15 A a; 16 B const& b = a.getB(); 17 B& c = (B&)b; 18 c.ptr = NULL; // OH NOES!!! 19 20 getch(); 21 return 0; 22}

Yes. People actually do this crap. :-/

Karadoc ~~
Member #2,749
September 2002
avatar

To me, that example demonstrates that casts are dangerous. I don't think it demonstrates that returning a reference to something internal is bad.

-----------

Slartibartfast
Member #8,789
June 2007
avatar

Stas B. said:

Yes. People actually do this crap. :-/

Sure you can cast away constness, but you can also overwrite any memory you want and do whatever, so I'm not sure what your point is.

#SelectExpand
1class B { 2 public: 3 char* ptr; 4}; 5 6class A { 7 B b; 8 public: 9 10 B const& getB() { return b; } 11}; 12 13int main() 14{ 15 A a; 16 int * ptr = (int *)(&a); 17 (*ptr) = 9001; //Oh ears! 18 return 0; 19}

Additionally, why is char *ptr public? (not that it would protect you as you could just int * c = (int *)&b; (*c) = 0;<code> or something similar.

someone972
Member #7,719
August 2006
avatar

The way I look at it is that const correctness is more to prevent the programmer from making stupid mistakes by warning them that they are trying to modify something that shouldn't be modified. If the programmer then decides to do something stupid and start cast-hacking then it's just bad practice on their part, and it will probably come back to bite them eventually. At least that's how I see it; the formal definition is probably different.

______________________________________
As long as it remains classified how long it took me to make I'll be deemed a computer game genius. - William Labbett
Theory is when you know something, but it doesn't work. Practice is when something works, but you don't know why. Programmers combine theory and practice: Nothing works and they don't know why. -Unknown
I have recklessly set in motion a chain of events with the potential to so-drastically change the path of my life that I can only find it to be beautifully frightening.

Stas B.
Member #9,615
March 2008

Data members of your class are implementation details. Returning references to them means exposing implementation details. You break encapsulation for no good reason.

Let's assume that the OP writes an Allegro game using a Keyboard class that returns a reference to a key-state vector. If for whatever reason he needs to port his game to another library that only has a function to check key states, he will be stuck with his irrelevant key-state vector. If instead he implements a 'getKeyState' member function, he may transparently re-implement it. Having the high-level keyboard wrapper mimic Allegro-specific implementation details completely defeats the purpose of having it in the first place.

Besides, your counter-arguments that you can break just about anything with explicit casting are straw-men... Normal people don't usually get tempted to break things just because they can, but they do get tempted to use nasty hacks when they think they can get away with it.

Karadoc ~~
Member #2,749
September 2002
avatar

Stas B. said:

Let's assume that the OP writes an Allegro game using a Keyboard class that returns a reference to a key-state vector. If for whatever reason he needs to port his game to another library that only has a function to check key states, he will be stuck with his irrelevant key-state vector. If instead he implements a 'getKeyState' member function, he may transparently re-implement it. Having the high-level keyboard wrapper mimic Allegro-specific implementation details completely defeats the purpose of having it in the first place.

This is a good argument.

Quote:

Besides, your counter-arguments that you can break just about anything with explicit casting are straw-men... Normal people don't usually get tempted to break things just because they can, but they do get tempted to use nasty hacks when they think they can get away with it.

I don't think the counter-arguments were straw-men arguments. A straw-man argument when one argues against a similar but weaker position under the pretence that they are actually arguing against the original position. I don't see any straw-men here. We* are just saying that its actually normal for stuff to be breakable using casts and other similar hacks, and so it can't really be blamed on the reference. I think you made a good argument for why the keyboard thing shouldn't be returned as a reference to a vector, but I don't think the same can be said for all would-be-reference-returning functions.

-----------

Stas B.
Member #9,615
March 2008

Karadoc said:

I think you made a good argument for why the keyboard thing shouldn't be returned as a reference to a vector...

And I thought I made a good argument for why you shouldn't expose internally-used objects that may cease existing for unforeseen reasons in the future. :-/
You just shouldn't break encapsulation without a compelling reason, even if you believe at the moment that the chances of it ever biting you are nil. I'm not saying you should never do it. I'm just saying you shouldn't do it by default when you have better choices.

And your arguments are straw-men because you pretend that my position is that code should be 100% fool-proof and protect masochists from themselves. In reality, all I'm saying is that you shouldn't write code that's easily breakable by doing something that may superficially make sense. Especially when it costs you next to nothing to make the world a saner place.

Karadoc ~~
Member #2,749
September 2002
avatar

I just opened some code that I've written to see if I ever return a reference to internally-used objects. Here's an example of what I found.

const std::string& GetName(void) const;
const Attack& GetAttack(void) const;

To me, those functions seem fine. Both of them basically just return a reference to an internal variable. (Sometimes they return a reference to something else, for example the "attack" of the weapon the character is holding, or whatever.) To me it just seems completely natural and sensible to 'expose the internals' in this case - but still, some bad stuff could happen if someone decided that the const wasn't important...

-----------

Stas B.
Member #9,615
March 2008

The first function is perfectly valid.
Can't say much about the second one, but it looks a bit fishy. :P
In any case, I'm not trying to profess any "never do" or "always do" rules. Such rules don't exist. I'm just saying you should think before you do it. Sometimes it's just plain bad. Sometimes it's the most sensible thing to do. Most of the time, it's just not optimal.

Slartibartfast
Member #8,789
June 2007
avatar

Stas B. said:

Let's assume that the OP writes an Allegro game using a Keyboard class that returns a reference to a key-state vector. If for whatever reason he needs to port his game to another library that only has a function to check key states, he will be stuck with his irrelevant key-state vector. If instead he implements a 'getKeyState' member function, he may transparently re-implement it. Having the high-level keyboard wrapper mimic Allegro-specific implementation details completely defeats the purpose of having it in the first place.

Okay, now I understand what you were getting at. In which case I guess it all depends on what exactly you mean with "internally used objects".
Would it make sense to return a reference to one member in the key vector? That is still an internally used object.
Would it make sense to return iterators over the vector? It might, and would still be a reference to internally used objects.
I guess the real "truth" of the matter is that you don't want to reference implementation details, only the details given in the agreed upon interface; And if that interface specifies a vector to an array of keys (for whatever reason) then I see nothing wrong with returning a reference to the internal vector.

Stas B. said:

And your arguments are straw-men because you pretend that my position is that code should be 100% fool-proof and protect masochists from themselves. In reality, all I'm saying is that you shouldn't write code that's easily breakable by doing something that may superficially make sense. Especially when it costs you next to nothing to make the world a saner place.

In reality if you cast away constness (except for extreme cases and with my consultation) you should be fired in much the same way you would be fired for directly referring to the structure of something in memory (which is also excusable and even useful in some cases).
IMO these two "illegal" actions are both similarly illegal, and if you allow the first then you should allow the second.

Stas B.
Member #9,615
March 2008

Would it make sense to return a reference to one member in the key vector? That is still an internally used object.

No. It wouldn't make sense because said vector may cease to exist in future implementations.

Slartibartfast said:

Would it make sense to return iterators over the vector? It might, and would still be a reference to internally used objects.

No. It wouldn't make sense because said vector may cease to exist in future implementations.

Slartibartfast said:

I guess the real "truth" of the matter is that you don't want to reference implementation details, only the details given in the agreed upon interface; And if that interface specifies a vector to an array of keys (for whatever reason) then I see nothing wrong with returning a reference to the internal vector.

By your logic, you can solve the problem posed by my example by exclaiming loudly "No! The vector is not irrelevant! It's the agreed upon interface! Deal with it!"
The entire point of an interface is being abstract. If your interface mandates the use of a vector to an array of keys, then you've got a shitty interface. ::)

Slartibartfast
Member #8,789
June 2007
avatar

Stas B. said:

No. It wouldn't make sense because said vector may cease to exist in future implementations.

So then what do I return in GetKeyState()?

Quote:

No. It wouldn't make sense because said vector may cease to exist in future implementations.

It wouldn't cease to exist in future implementations because the interface specifies we return a vector. Getting rid of the vector would break the interface.

Quote:

By your logic, you can solve the problem posed by my example by exclaiming loudly "No! The vector is not irrelevant! It's the agreed upon interface! Deal with it!"

Sure I can, if by definition all implementations of the interface have to return an array of keys, then that is something I can rely on to stay constant in all implementations, whether they use Allegro or SDL.

Quote:

The entire point of an interface is being abstract. If your interface mandates the use of a vector to an array of keys, then you've got a shitty interface.

How is returning an array of keys not abstract? How is it shitty?

Stas B.
Member #9,615
March 2008

What do you return in GetKeyState? The state of the key. Not a reference to a member of a vector. I guess we're just arguing about semantics at this point. You can call your key-state array anything you like. The bottom line is that you'll be stuck with it, so you better think 10 times before you declare it to be a part of your interface. I've already demonstrated why it's bad in practice. While a key vector may become a part of your interface because you defined it so, a GetKeyState member function should be a part of your interface because that's what a request to check the state of a key boils down to. I don't care about your key vector. I care about the state of the key. That's what abstraction is about.

Tobias Dammers
Member #2,604
August 2002
avatar

Stas B. said:

In fact, returning references to objects belonging to an instance of your class is generally a horrible idea since you virtually can't protect them from being modified in unintended ways.

Please re-read my post. I never implied you should be returning a reference to a class internal. In fact, what I recommend is the pragmatic approach where class Keyboard simply has a getter for one key state. But even with the more elaborate solution, I did not imply any specific technical implementation of the pattern - all I said was that you could return an object that is responsible for getting keyboard states.

Obviously, if you're using C++, returning a non-const reference to a class's internals is usually a bad idea, if only because it is one of the rare situations where you can accidentally produce invalid references:

Foobar* foo = new Foobar();
Baz& baz(foo.getBaz());
delete foo;
baz.quux(); // boom!

However, there are other possible implementations:

  • class KeyStates queries the underlying keyboard API directly

  • class KeyStates contains an immutable collection of key states

  • class Keyboard is managed through some kind of GC system (ref counting, or whatever is feasible), and will only get deleted once all associates KeyStates objects go out of scope; this way, it is perfectly safe for class KeyStates to call back into its parent Keyboard instance

  • class Keyboard is a singleton or monostate, and class KeyStates uses appropriate mechanisms to call back into it

  • etc. etc. etc.

In fact, the reference problem is only a real problem if you're naively using a language without built-in GC; C#, Java, D, Python, and a bunch more, will allow for these things without any hassle whatsoever.

Also note that this solution does not violate encapsulation at all - the KeyStates class adheres to the "do one thing" philosophy, and allows for a very narrow interface, providing nothing but read-only access to a snapshot of key states. By contrast, if you pass an instance class Keyboard into a function, that function can do EVERYTHING the Keyboard class exposes - read key states, change modifier states, set LED status, change keyboard layout, etc.; OTOH, if you only pass an immutable KeyStates object, the function can do nothing but read key states.

---
Me make music: Triofobie
---
"We need Tobias and his awesome trombone, too." - Johan Halmén

 1   2 


Go to: