Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have a MouseInputHandler class that registers itself to an InputManager. When the InputManager class receives events they are forwarded to MouseInputHandler which changes it's state to reflect the state of the mouse. A mouse as an input device provides a large number of different properties that should be easy to read, but impossible to write to.

The problem is that C++ fails to provide a keyword otherwise similar to const but that grants full write access to functions in the same scope.

class MouseInputHandler : public InputHandler
{
public:
    MouseInputHandler(ResourceContext &context)
        : btnLeft(btnLeft_), btnMiddle(btnMiddle_), btnRight(btnRight_),
          btnX1(btnX1_), btnX2(btnX2_), absX(absX_), absY(absY_), relX(relX_),
          relY(relY_), scrollX(scrollX_), scrollY(scrollY_) {}
    void handleEvent(SDL_Event &mouseEvent);
    void refresh();

    const Signal &btnLeft, &btnMiddle, &btnRight, &btnX1, &btnX2;
    const int32_t &absX, &absY, &relX, &relY, &scrollX, &scrollY;
protected:
    Signal btnLeft_, btnMiddle_, btnRight_, btnX1_, btnX2_;
    int32_t absX_, absY_, relX_, relY_, scrollX_, scrollY_;
};

Options I have identified at this point:

  1. Declaring the members private or protected and providing public const references to the non-public members that only allow read access (as in the example above). However with 11 members, the class quicly becomes a mess, with 11 const references (that also require 11 reference initializations in the constructor's initializer list).
  2. Adding getter methods for every member variable. More cumbersome than the const references, but with the (marginal) added benefit that the members are now accessible as functions.
  3. Placing the buttons and integers in (private or protected) arrays and adding an enum that gives indexes to the array elements meaningful names. To access the members, two (one getter for buttons, other for ints) getter functions that take the index and return a const reference are added to the class. This seems to me quite complex and error prone in comparison to the other solutions.
  4. Just not caring about const correctness at all and making the member variables fully accessible.
  5. Refractoring the code. I don't see how one could refractor a mouse, or simplify the interface, but any suggestions are welcome.
share|improve this question
    
I went with the struct + accessor method approach as suggested by ChrisW. Placing the variables separately did not come to my mind for some reason. –  user1062874 Feb 21 at 19:13
    
I think the keyword you are looking for is called "friend", although it is often said you shouldn't make use of it. –  Alexis Wilke Feb 21 at 22:46
    
@AlexisWilke "friend" is to get past a "private" barrier (i.e. no access at all); I don't see how to use it to help implement and/or get past a const-versus-non-const barrier. –  ChrisW Feb 22 at 0:11
    
@ChrisW, note that this comment was in link with the OP sentence: "The problem is that C++ fails to provide a keyword otherwise similar to const but that grants full write access to functions in the same scope." -- a class can allow another to write to its fields even if those are private. Of course, that doesn't remove the "const-ness" but marking variables as const when in reality they are not is anyway misleading. –  Alexis Wilke Feb 22 at 0:50
    
@AlexisWilke I still don't see how "friend" could be used in an answer to the question. –  ChrisW Feb 22 at 0:52
show 1 more comment

3 Answers

up vote 3 down vote accepted

Perhaps something like this:

struct MouseSettings
{
    Signal btnLeft_, btnMiddle_, btnRight_, btnX1_, btnX2_;
    int32_t absX_, absY_, relX_, relY_, scrollX_, scrollY_;
}

class MouseInputHandler : public InputHandler
{
public:
    MouseInputHandler(ResourceContext &context)
        : mouseSettings(mouseSettings_)
    {}
    void handleEvent(SDL_Event &mouseEvent);
    void refresh();

    const MouseSettings& mouseSettings;

protected:
    MouseSettings mouseSettings_;
};

Or a property-like get-accessor method like:

const MouseSettings& getMouseSettings() { return mouseSettings_; }
share|improve this answer
add comment

As a side note, getters are fully optimized by the compiler, whereas the cont references are not (read as cannot). References generate at least 2 instructions because the reference is a pointer which cannot safely be removed by the compiler (because you can change those pointers over time):

class F
{
    int member;
public:
    cont int& member_;
};

x = f.member; // this is a full reference, so a pointer to a value:
              // mov [%eax+4], %ebx + mov [%ebx], %ebx

When a simple getter as we see here is fully optimized and get one a faster program with the security you are looking for:

class G
{
    int member_;
public:
    int get_member() const { return member_; }
};

x = f.get_member(); // this gets optimized to one load: mov [%eax], %ebx

This last example becomes exactly the same as:

x = f.member_;

but is read-only (and allows you to write more code in the getter as required with time.)

Another drawback with the const reference, you can overrule the constness with a cast:

const_cast<int&>(f.member_) = 5;

So it may not be as safe as you think.

In regard to ChrisW's answer, the last entry has the same potential problem with the const_cast<>() capability. Although of course, you have to work at it to want to write data in the wrong place...

const MouseSettings& getMouseSettings() { return mouseSettings_; }
const_cast<MouseSettings&>(f.getMouseSettings()).absX_ = 123;

This is rarely an issue since pretty much no one wants to do such. However, if you work with multiple threads, that write issue is a problem because now you have a reference to a "moving" target. So a safer implementation is:

MouseSettings getMouseSettings() { return mouseSettings_; }

and it gets optimized like a standard getter, so:

x = f.getMouseSettings().absX_;

is one single mov instruction as if you had just one get_absX() const function (see above).

Now, to answer your question some more, I'm not too sure where your problem is in your current implementation since the only thing that can modify the values defined in the class is the handleEvent() function and that function will have full write permissions.

void handleEvent(SDL_Event &mouseEvent)
{
    f.member_ = 123; // this works here!
}

So for the writer I would rewrite the handleEvent() function like this:

class InputHandler
{
...
protected:
    virtual void handleEvent(...) = 0; // force all derived class to handle events
};

class MouseInputHandler : public InputHandler
{
...
protected:
    virtual void handleEvent(...) { ... };
};

The body of the handleEvent() could of course be written in the .cpp file. This means on the InputHandle has the capability to write to the mouse input handler variables in a rather save manner.

share|improve this answer
    
Returning a copy of the value not only more expensive but is also insufficient protection: because a malicious caller can reinterpret_cast MouseInputHandler* to char* and then poke into private member data. –  ChrisW Feb 21 at 23:31
    
@ChrisW, when you return a copy, you can modify the copy. But not the content of the source in the class. A reinterpret_cast<>() won't help you on the copy returned. However if you cast the class object itself, of course. Then you can do anything anyway... C/C++ are dreadful in that sense. However, the compiler knows how to optimize a value returned by copy and transform it to a single mov instruction in assembly. (assuming the getter is written inline.) –  Alexis Wilke Feb 21 at 23:37
    
+1 for implying that if the program is multi-threaded then you'd better "lock, return a copy of the whole structure, unlock" instead of returning a reference to volatile data: because that gives the caller a copy of a coherent set of data. –  ChrisW Feb 22 at 0:50
    
Your assertions about references being pointers is false. The compiler can just as easily remove them as inline getters. When writting getters, best to implement as returning a const reference to avoid the copy. Code with const_cast<> is dicey at best and casting it away in this situation would be undefined behavior. So you are offering a false analogy as an argument for not using them. Though I agree that getter is the way to go your arguments for doing so are mostly wrong. –  Loki Astari Feb 22 at 16:41
    
I tested with a simple case and everything inline and g++ optimizes everything properly. So I guess that doesn't stand anymore. –  Alexis Wilke Feb 23 at 5:24
add comment

Declaring the members private or protected and providing public const references to the non-public members that only allow read access (as in the example above). However with 11 members, the class quicly becomes a mess, with 11 const references (that also require 11 reference initializations in the constructor's initializer list).

protected is a bad option as it does not really provide any kind of protections. It is to easy for a developer a couple of years from now deciding that he wants to have access and just inherit from your class to get the access he wants. Thus violating constraints that you probably failed to document. So prefer never to use protected. Stick with either fully private or fully public.

Adding reference variable may potentially double the size of your class if the compiler is having a bad day and does not optimize them out. Also this adds restictions on the copy-ability of your class (the default copy constructor is not going to do the expected thing).

Adding getter methods for every member variable. More cumbersome than the const references, but with the (marginal) added benefit that the members are now accessible as functions.

Adding getters for each member would become cumbersome. But you could start doing some template magic to remove repetitive code.

Placing the buttons and integers in (private or protected) arrays and adding an enum that gives indexes to the array elements meaningful names. To access the members, two (one getter for buttons, other for ints) getter functions that take the index and return a const reference are added to the class. This seems to me quite complex and error prone in comparison to the other solutions.

I disagree. That seems like a fairly reasonable solution:

enum T {absX_, absY_, relX_, relY_, scrollX_, scrollY_};
enum S {btnLeft_, btnMiddle_, btnRight_, btnX1_, btnX2_};
class LotsOfPrivateMembers
{
     int     intData[6];
     double  signalData[5];

     public:

         template<T i>
         int const& get() { return intData[i];}
         template<S i>
         int const& get() { return signalData[i];}
};

Just not caring about const correctness at all and making the member variables fully accessible.

Not a good idea.
Make sure your code is const correct is a big part of modern C++.

Refractoring the code. I don't see how one could refractor a mouse, or simplify the interface, but any suggestions are welcome.

Suggestion above.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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