How do I remove code duplication between similar const and non-const member functions?

Question

For a detailed explanation, please see the heading "Avoid Duplication in const and Non-const Member Function," on p. 23, in Item 3 "Use const whenever possible," in Effective C++, 3d ed by Scott Meyers, ISBN-13: 9780321334879.

Here's Meyers' solution (simplified):

struct C {
  const char & get() const {
    return c;
  }
  char & get() {
    return const_cast<char &>(static_cast<const C &>(*this).get());
  }
  char c;
};

The two casts and function call may be ugly, but it's correct in a non-const method as that implies the object was not const to begin with. (Meyers has a thorough discussion of this.)

Yes, it is possible to avoid the code duplication. You need to use the const member function to have the logic and have the non-const member function call the const member function and re-cast the return value to a non-const reference (or pointer if the functions returns a pointer):

class X
{
   std::vector<Z> vecZ;

public:
   const Z& z(size_t index) const
   {
      // same really-really-really long access 
      // and checking code as in OP
      // ...
      return vecZ[index];
   }

   Z& z(size_t index)
   {
      // One line. One ugly, ugly line - but just one line!
      return const_cast<Z&>( static_cast<const X&>(*this).z(index) );
   }

 #if 0 // A slightly less-ugly version
   Z& Z(size_t index)
   {
      // Two lines -- one cast. This is slightly less ugly but takes an extra line.
      const X& constMe = *this;
      return const_cast<Z&>( constMe.z(index) );
   }
 #endif
};

NOTE: It is important that you do NOT put the logic in the non-const function and have the const-function call the non-const function -- it may result in undefined behavior. The reason is that a constant class instance gets cast as a non-constant instance. The non-const member function may accidentally modify the class, which the C++ standard states will result in undefined behavior.

C++17 has updated the best answer for this question:

T const & f() const {
    return something_complicated();
}
T & f() {
    return const_cast<T &>(std::as_const(*this).f());
}

This has the advantages that it:

• Is obvious what is going on
• Has minimal code overhead -- it fits in a single line
• Is hard to get wrong (can only cast away volatile by accident, but volatile is a rare qualifier)

If you want to go the full deduction route then that can be accomplished by having a helper function

template<typename T>
constexpr T & as_mutable(T const & value) noexcept {
    return const_cast<T &>(value);
}
template<typename T>
constexpr T * as_mutable(T const * value) noexcept {
    return const_cast<T *>(value);
}
template<typename T>
constexpr T * as_mutable(T * value) noexcept {
    return value;
}
template<typename T>
void as_mutable(T const &&) = delete;

Now you can't even mess up volatile, and the usage looks like

decltype(auto) f() const {
    return something_complicated();
}
decltype(auto) f() {
    return as_mutable(std::as_const(*this).f());
}

I think Scott Meyers' solution can be improved in C++11 by using a tempate helper function. This makes the intent much more obvious and can be reused for many other getters.

template <typename T>
struct NonConst {typedef T type;};
template <typename T>
struct NonConst<T const> {typedef T type;}; //by value
template <typename T>
struct NonConst<T const&> {typedef T& type;}; //by reference
template <typename T>
struct NonConst<T const*> {typedef T* type;}; //by pointer
template <typename T>
struct NonConst<T const&&> {typedef T&& type;}; //by rvalue-reference

template<typename TConstReturn, class TObj, typename... TArgs>
typename NonConst<TConstReturn>::type likeConstVersion(
   TObj const* obj,
   TConstReturn (TObj::* memFun)(TArgs...) const,
   TArgs&&... args) {
      return const_cast<typename NonConst<TConstReturn>::type>(
         (obj->*memFun)(std::forward<TArgs>(args)...));
}

This helper function can be used the following way.

struct T {
   int arr[100];

   int const& getElement(size_t i) const{
      return arr[i];
   }

   int& getElement(size_t i) {
      return likeConstVersion(this, &T::getElement, i);
   }
};

The first argument is always the this-pointer. The second is the pointer to the member function to call. After that an arbitrary amount of additional arguments can be passed so that they can be forwarded to the function. This needs C++11 because of the variadic templates.

Nice question and nice answers. I have another solution, that uses no casts:

class X {

private:

    std::vector<Z> v;

    template<typename InstanceType>
    static auto get(InstanceType& instance, std::size_t i) -> decltype(instance.get(i)) {
        // massive amounts of code for validating index
        // the instance variable has to be used to access class members
        return instance.v[i];
    }

public:

    const Z& get(std::size_t i) const {
        return get(*this, i);
    }

    Z& get(std::size_t i) {
        return get(*this, i);
    }

};

However, it has the ugliness of requiring a static member and the need of using the instance variable inside it.

I did not consider all the possible (negative) implications of this solution. Please let me know if any.

A bit more verbose than Meyers, but I might do this:

class X {

    private:

    // This method MUST NOT be called except from boilerplate accessors.
    Z &_getZ(size_t index) const {
        return something;
    }

    // boilerplate accessors
    public:
    Z &getZ(size_t index)             { return _getZ(index); }
    const Z &getZ(size_t index) const { return _getZ(index); }
};

The private method has the undesirable property that it returns a non-const Z& for a const instance, which is why it's private. Private methods may break invariants of the external interface (in this case the desired invariant is "a const object cannot be modified via references obtained through it to objects it has-a").

Note that the comments are part of the pattern - _getZ's interface specifies that it is never valid to call it (aside from the accessors, obviously): there's no conceivable benefit to doing so anyway, because it's 1 more character to type and won't result in smaller or faster code. Calling the method is equivalent to calling one of the accessors with a const_cast, and you wouldn't want to do that either. If you're worried about making errors obvious (and that's a fair goal), then call it const_cast_getZ instead of _getZ.

By the way, I appreciate Meyers's solution. I have no philosophical objection to it. Personally, though, I prefer a tiny bit of controlled repetition, and a private method that must only be called in certain tightly-controlled circumstances, over a method that looks like line noise. Pick your poison and stick with it.

[Edit: Kevin has rightly pointed out that _getZ might want to call a further method (say generateZ) which is const-specialised in the same way getZ is. In this case, _getZ would see a const Z& and have to const_cast it before return. That's still safe, since the boilerplate accessor polices everything, but it's not outstandingly obvious that it's safe. Furthermore, if you do that and then later change generateZ to always return const, then you also need to change getZ to always return const, but the compiler won't tell you that you do.

That latter point about the compiler is also true of Meyers's recommended pattern, but the first point about a non-obvious const_cast isn't. So on balance I think that if _getZ turns out to need a const_cast for its return value, then this pattern loses a lot of its value over Meyers's. Since it also suffers disadvantages compared to Meyers's, I think I would switch to his in that situation. Refactoring from one to the other is easy -- it doesn't affect any other valid code in the class, since only invalid code and the boilerplate calls _getZ.]