0
\$\begingroup\$

For a little game of mine I have created a Key.h file that should allow representation of a key press combination. A key combination is basically some key represented by its virtual key code stored in int _keyand the state of modifier keys - shift, control, alt and windows keys. In case that key combination is used for typing, the key also stores the character that it should produce in _c.

class Key
{
private:
    bool _shift;
    bool _ctrl;
    bool _alt;
    bool _win;
    int _key;
    char _c;

public:
    constexpr Key()
        :
        _shift(),
        _ctrl(),
        _alt(),
        _win(),
        _key(),
        _c()
    {}

    constexpr Key(const Key& key)
        :
        _shift(key._shift),
        _ctrl(key._ctrl),
        _alt(key._alt),
        _win(key._win),
        _key(key._key),
        _c(key._c)
    {}

    constexpr Key(bool shift, bool ctrl, bool alt, bool win, int key, char c)
        :
        _shift(shift),
        _ctrl(ctrl),
        _alt(alt),
        _win(win),
        _key(key),
        _c(c)
    {}

    Key& operator= (Key& key)
    {
        _shift = key._shift;
        _ctrl = key._ctrl;
        _alt = key._alt;
        _win = key._win;
        _key = key._key;
        _c = key._c;
    }

    bool operator== (const Key& key) const
    {
        return (_shift == key._shift && _ctrl == key._ctrl && _alt == key._alt && _win == key._win && _key == key._key, _c == key._c);
    }

    bool operator!= (const Key& key) const
    {
        return !(*this == key);
    }
};

Are there any design flaws or some obvious things that I'm missing?

\$\endgroup\$
2
\$\begingroup\$

I would start by fixing the warnings, one of which is an actual bug:


[1] Operator = not returning a value

main.cpp: In member function 'Key& Key::operator=(Key&)':
main.cpp:50:5: warning: no return statement in function returning non-void [-Wreturn-type]
     }
     ^
main.cpp: In member function 'bool Key::operator==(const Key&) const':

Also: your operator= gets Key& key instead of const Key& key

Wait! don't rush to fix it. You can just eliminate it altogether and rely on the default assignment operator that does the same job ("rule of zero").


[2] using comma in the long list of equality check -- an actual bug

main.cpp:54:100: warning: left operand of comma operator has no effect [-Wunused-value]
         return ( ... && _win == key._win && _key == key._key, _c == key._c);
        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~

Wait! don't rush to fix it. We will use std::bitset so the entire operator== would become simpler.


After fixing the warnings above we can proceed for improvements.

[3] Use std::bitset for the bool flags

Instead of:

bool _shift;
bool _ctrl;
bool _alt;
bool _win;

Use:

enum SpecialKeys {SHIFT, CTRL, ALT, WIN, _SIZE_};
std::bitset<SpecialKeys::_SIZE_> specialKeys;

[4] No need for assignment operator and copy ctor (rule of zero).

Just use the defaults.


Proposed fixed code:

#include <bitset>

class Key {
    enum SpecialKeys {SHIFT, CTRL, ALT, WIN, _SIZE_};
    std::bitset<SpecialKeys::_SIZE_> specialKeys;
    int _key = 0;
    char _c = 0;
public:
    constexpr Key() {}
    constexpr Key(bool shift, bool ctrl, bool alt, bool win, int key, char c)
        : _key(key), _c(c)
    {
        specialKeys[SpecialKeys::SHIFT] = shift;
        specialKeys[SpecialKeys::CTRL]  = ctrl;
        specialKeys[SpecialKeys::ALT]   = alt;
        specialKeys[SpecialKeys::WIN]   = win;
    }

    bool operator== (const Key& key) const
    {
        return (specialKeys == key.specialKeys && _key == key._key && _c == key._c);
    }

    bool operator!= (const Key& key) const
    {
        return !(*this == key);
    }
};
\$\endgroup\$
  • \$\begingroup\$ Ah, very constructive. Since you mentioned the rule of zero, should I just completely remove the copy constructor or request it from compiler with = default ? \$\endgroup\$ – Mantas Kandratavicius Jan 14 at 2:26
  • \$\begingroup\$ Just use the defaults by not declaring at all, no need for = default in this case. \$\endgroup\$ – Amir Kirsh Jan 14 at 2:32
  • \$\begingroup\$ Don't use _SIZE_ as an identifier in your code - that's reserved for the implementation for any use (so could even be a macro). \$\endgroup\$ – Toby Speight Jan 14 at 14:57
  • \$\begingroup\$ I like this version more, in the upcoming C++20, you would even be able to default the operator<=> \$\endgroup\$ – JVApen Jan 15 at 7:27

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.