# Avoiding code duplication and retaining const correctness

Sometimes I run across the problem of having two member function overloads that only differ in the constness of this (and the return type, but that is less important):

int& operator[](int);
const int& operator[](int)const;


Most of the time these are one-liners, but even then, I hate the code duplication. That's why I like to use (if possible) the const version to implement the non const version via const_cast:

const int& MyClass::operator[](int index)const {
return const_cast<int&>(const_cast<MyClass&>(*this)[index]);
}


The first const_cast is required (at least I think so), but I wonder if the second one can be circumvented and if so, if the code is more readable then.

How do you solve this problem?

-
There are several good answers on StackOverflow: see How do I remove code duplication between similar const and non-const member functions? –  ChrisW Mar 15 '14 at 19:30

First you have it the wrong way around.

You should not use the mutating version to supply data to the non-mutating function:

const int& MyClass::operator[](int index)const {
return const_cast<int&>(const_cast<MyClass&>(*this)[index]);
}


What happens if operator[] is modified and does mutate slightly the internal state? By casting away the constness you have opened up your class to undefined behavior. You should NEVER cast away constness. If you must you can use const_cast to add constness, but removing it is so dangerous that this should be looked at as breaking code.

If you had written it the other way around:

int& MyClass::operator[](int index) {
return const_cast<int&>(const_cast<MyClass const&>(*this)[index]);
}


This is better. Because if you change the operator[] so that it actually mutates the class it will generate a compiler error and thus prevent you from accidentally getting undefined behavior.

But personally I prefer to add another helper method and thus remove all athe casting:

private:
// NOTE: This is not a perfect solution for all situations.
//       But it works for 99% of situations and keeps the code clean
//       For the remaining situations switch back to use const cast
//       **BUT** always add never remove constness from an object.
// Never make public as it is const and gives away an internal reference.
// Only use via the public API
int& get(int index) const
{
return stuff[index];
}

public:
// Keep the API simple/clean and DRY
// move all the work to private methods.
int&       operator[](int index)       { return get(index);}
int const& operator[](int index) const { return get(index);}


I would not use it for const/non-const iterators

const_iterator begin() const {return const_iterator(<X>);}
iterator       begin()       {return iterator(<X>);}

-
Would you show how that works if get is supposed to return a const or non-const iterator type, instead of a const or non-reference? Perhaps get return a non-const iterator instance, and you define the const iterator type so that it can be explicitly or implicitly created from a non-const iterator instance? –  ChrisW Mar 15 '14 at 22:46
@ChrisW: Hope the edit helps. –  Loki Astari Mar 16 '14 at 1:02
It seems you have misplaced the ( in your comment answer. @ChrisW: As the code in question is usually about own code, you can write your own const_cast that converts the iterators. If you are dealing with (automatically) const standard contains in C++11 you can use the erase function to convert const iterators to mutable iterators in O(1). –  Nobody Mar 17 '14 at 9:39

As ChrisW commented, this has been covered in depth at Stack Overflow. You're very close, but you should actually have the non-const version cast away the const-ness of the const version rather than how you're doing it.

If you're calling a non-const method, you are operating on a non-const object. This means that the const cast is technically valid since you are assured that the underlying object is non-const anyway.

Note that there are other options that are potentially cleaner from a "I should always avoid const_cast!" point of view. Personally, I think this is one of the legitimate uses of const_cast, and it is the approach I use. If you'd like to see more information on the other options, see the Stack Overflow post.

-
Hm I just reproduced the code from the top of my head and did not notice I had done it the other way around. Of course you are right. The non const code could do manipulations that we don't want the const code to suffer from. –  Nobody Mar 15 '14 at 20:55

One (C++11) solution is to "hide" the ugly second const_cast with a macro (shudder):

#include <type_traits>
#define CONST_THIS const_cast< \
std::remove_pointer< \
decltype(this)>::type>::type>::type>(this)


And use it like this:

int& operator[](int index) {
return const_cast<int&>((*CONST_THIS)[index]);
}


The code in the macro uses type traits to add constness to the this pointer. To do so it must at first strip the pointer (to apply the const to the pointed to type) and then make it a pointer again to use the macro as if it where the this pointer in a const member function.

-
Does any of the downvoters mind to give an explanation? I dislike macros but I find the code more readable that way and it certainly improves over the "useless" repetition of the type of this to use the const_cast. –  Nobody Mar 17 '14 at 9:42