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

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)

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

key->button(ALLEGRO_KEY_UP);

Why not overload operator [] instead?

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

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?

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.

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.

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

Then return a reference to a const?

O rly?

Yes. People actually do this crap.

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

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.

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.

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.

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.

This is a good argument.

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.

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.

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...

The first function is perfectly valid.
Can't say much about the second one, but it looks a bit fishy.
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.

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.

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.

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

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

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.

So then what do I return in GetKeyState()?

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.

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.

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

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.

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.

Returning a reference to a const should be a contract between the caller and object that the referenced object will persist for at least as long as the returning object itself and that the caller is not to modify it. So long as this contract is upheld then there's no harm. Even better if the returned object is immutable.

I would agree that an object that exposes a keyboard state should support an interface to check the state of a key rather than returning the internal state. You might offer a way to query multiple keys at a time, but I would prefer that interface to return a copy of internal state.

void Keyboard::getKeyStates(std::map<int,bool> & in_out) const
{
    for(std::map<int,bool>::iterator it = in_out.begin();
            it != in_out.end();
            it++)
    {
        int key = it->first;

        results[key] = this->getKeyState(key);
    }
}

That's actually a pretty sound solution. I guess I misunderstood your post.

No harm done