Const function calling non const or vice versa (to avoid duplication)?

Question

Is there any advantage using one over the other:

class Foo
{
public:
    const int& get() const
    {
        // stuff here
        return myInt;
    }

    int& get()
    {
        return const_cast<int&>(static_cast<const Foo*>(this)->get());
    }
};

Or

class Foo
{
public:
    int& get()
    {
        // stuff here
        return myInt;
    }

    const int& get() const
    {
        return const_cast<Foo*>(this)->get();
    }
};

I only used the first one, but I just saw the second one used somewhere, so I am wondering.

The comment // stuff here could be a non-trivial check like retrieving the index of a table in order to return a ref on a member of the table (for example: myInt = myTable[myComputedIndex];) so I cannot just make it public. Thus table and any member are not const.

Answer 1

If you have to make a function that is const-agnostic, and avoids duplication, one neat way to do it is delegating implementation to a template, for example

class Foo {
private: 

    int my_int;
    template <typename ThisPtr>
    static auto& get(ThisPtr this_ptr) { 
        return this_ptr->my_int;
    }

public:
    int& get() {
        return get(this);
    }

    const int& get() const {
        return get(this);
    }
};

This way you are free from the fear associated with using const_cast, mutable and other stuff that goes into trying to reduce code duplication in cases like this. If you get something wrong, the compiler will let you know.

Answer 2

Ignoring the issue of whether you really need a getter, the best solution when duplicating functionality in both a const and non-const method is to have the non-const method call the const method and cast away the const-ness of the result (i.e. the first of the two alternatives you present in the question).

The reason is simple: if you do it the other way around (with the logic in the non-const method), you could accidentally end up modifying a const object, and the compiler won't catch it at compile time (because the method is not declared const) - this will have undefined behaviour.

Of course this is only a problem if the "getter" is not actually a getter (i.e. if it is doing something more complicated than just returning a reference to a private field).

Also, if you are not constrained to C++11, the template-based solution presented by Curious in their answer is another way of avoiding this problem.

Answer 3

Is there any advantage using one over the other: ...

No, both are bad because they violate the data encapsulation principle.

In your example you should rather make myInt a public member. There's no advantage to have getters for such case at all.

If you really want (need) getter and setter functions these should look like this:

class Foo
{
private: 
    mutable int myInt_;
 // ^^^^^^^ Allows lazy initialization from within the const getter,
 //         simply omit that if you dont need it.

public:
    void myInt(int value)
    {
        // Do other stuff ...
        myInt = value;
        // Do more stuff ...
    }

    const int& myInt() const
    {
        // Do other stuff ...
        return myInt_;
    }
}

Answer 4

You don't say where myInt comes from, the best answer depends on that. There are 2+1 possible scenarios:

1) The most common case is that myInt comes from a pointer internal to the class.

Assuming that, this is the best solution which avoids both code duplication and casting.

class Foo{
    int* myIntP;
    ... 
    int& get_impl() const{
       ... lots of code
       return *myIntP; // even if Foo instance is const, *myInt is not
    }
public:
    int& get(){return get_impl();}
    const int& get() const{return get_impl();}
};

This case above applies to pointer array, and (most) smart pointers.

2) The other common case is that myInt is a reference or a value member, then the previous solution doesn't work. But it is also the case where a getter is not needed at all. Don't use a getter in that case.

class Foo{
     public:
     int myInt; // or int& myInt;
};

done! :)

3) There is a third scenario, pointed by @Aconcagua, that is the case of an internal fixed array. In that case it is a toss-up, it really depends what you are doing, if finding the index is really the problem, then that can be factored away. It is not clear however what is the application:

class Foo{
    int myInts[32];
    ... 
    int complicated_index() const{...long code...}
public:
    int& get(){return myInts[complicated_index()];}
    const int& get() const{return myInts[complicated_index()];}
};

My point is, understand the problem and don´t over engineer. const_cast or templates are not needed to solve this problem.

---

complete working code below:

class Foo{
    int* myIntP;
    int& get_impl() const{
       return *myIntP; // even if Foo instance is const, *myInt is not
    }
public:
    int& get(){return get_impl();}
    const int& get() const{return get_impl();}

    Foo() : myIntP(new int(0)){}
    ~Foo(){delete myIntP;}
};

#include<cassert>

int main(){
    Foo f1; 
    f1.get() = 5;
    assert( f1.get() == 5 );

    Foo const f2;
//    f2.get() = 5; // compile error
    assert( f2.get() == 0 );    
    return 0;
}

Answer 5

As you intend access to more complex internal structures (as clarified via your edit; such as providing an operator[](size_t index) for internal arrays as std::vector does), then you will have to make sure that you do not invoke undefined behaviour by modifying a potentially const object.

The risk of doing so is higher in the second approach:

int& get()
{
    // stuff here: if you modify the object, the compiler won't warn you!
    // but you will modify a const object, if the other getter is called on one!!!
    return myInt;
}

In the first variant, you are safe from (unless you do const_cast here, too, which now would really be bad...), which is the advantage of this approach:

const int& get() const
{
    // stuff here: you cannot modify by accident...
    // if you try, the compiler will complain about
    return myInt;
}

If you actually need to modify the object in the non-const getter, you cannot have a common implementation anyway...

Answer 6

Modifying a const object through a non-const access path [...] results in undefined behavior.

(Source: http://en.cppreference.com/w/cpp/language/const_cast)

This means that the first version can lead to undefined behavior if myInt is actually a const member of Foo:

class Foo
{
    int const myInt;
public:
    const int& get() const
    {
        return myInt;
    }
    int& get()
    {
        return const_cast<int&>(static_cast<const Foo*>(this)->get());
    }
};
int main()
{
    Foo f;
    f.get() = 10; // this compiles, but it is undefined behavior
}

The second version would not compile, because the non-const version of get would be ill-formed:

class Foo
{
    int const myInt;
public:
    int& get()
    {
        return myInt;
        // this will not compile, you cannot return a const member
        // from a non-const member function
    }
    const int& get() const
    {
        return const_cast<Foo*>(this)->get();
    }
};
int main()
{
    Foo f;
    f.get() = 10;  // get() is ill-formed, so this does not compile
}

This version is actually recommended by Scott Meyers in Effective C++ under Avoid Duplication in const and Non-const Member Function.