Declaring a function const when changing member data

Question

Consider the following (example) code:

class A
{
private:
    int *_a;
public:
    A() { /* initialize _a to something */ }
    ~A() { /* deallocate _a */ }
    void setA(int i) const
    {
        _a[i] = 3;
    }
};

This code compiles and will perform as expected (i.e., if you call setA with some input i, it will set the ith element of _a to 3). My concern is with the const modifier attached to the function setA. There is no danger of a compiler error (or worse, undefined behavior) due to this modifier - the class A only contains a pointer to the data that is being modified, not the data itself. With that said, I can't help but feel that using the const modifier for a function that does in fact modify the data that A is in charge of maintaining is wrong, somehow. Am I being oversensitive here, or is this truly bad practice?

Answer 1

In your example it is probably very bad idea to modify _a[i].

Having said that I would like to elaborate a bit more:

const is a very useful keyword. If you read some Bjarne's or Scott's books, there is written to use const as often as possible. Moreover changing data in function declared const is not only possible, it is sometimes good practice! Just remember that care is needed when deciding if your case is one of those 'some' times. Why on Earth they would put keyword mutable in C++ if it should not be used?

An example (from one of aforementioned authors if I remember correctly) of good usage of mutable:

Consider class Polygon:

class Polygon
{
    void calculate_area() { /* we calculate m_area */ }

    std::vector<Vertex> m_vertexes;
    double m_area;

public:
    Polygon(std::initializer_list<Vertex> v_list): m_vertexes(v_list) {}

    double area() const { return m_area; }
    void add_vertex(Vertex v)
    {
        m_vertexes.push_back(v);
        calculate_area();
    }
};

It is quite straightforaward, isn't it? We don't want to calculate area every time we are asked to return it, so we store its value in m_area member variable and return this variable. Method double area() is const, it doesn't change anything after all. The thing is we have to compute area every time we change our Polygon! Let's say we add a hundred vertexes one by one... A hundred area recalculations! 99 of those totally unnecessary. We want to recalculate only if we are asked to deliver area. So what do we do?

We use mutable!

class Polygon
{
    void calculate_area() const
    {
        /* we calculate m_area */
        m_recalculate_area = false;
    }

    std::vector<Vertex> m_vertexes;
    mutable bool m_recalculate_area = false;
    mutable double m_area = 0.0;

public:
    Polygon(std::initializer_list<Vertex> v_list): m_vertexes(v_list)
    { m_recalculate_area = true; }

    double area() const
    {
        if(m_recalculate_area)
        { calculate_area(); }

        return m_area;
    }

    void add_vertex(Vertex v)
    {
        m_vertexes.push_back(v);
        m_recalculate_area = true;
    }
};

The effect is pretty nice: for a user of our class method double area() still doesn't modify object it's working on, so it is still const. But inside we gained a lot! Area will be recalculated only when there is need for it. If nobody asks for area it won't be calculated at all!

So, as I understand it, const should indicate that "as far as class user is concerned" method does not modify object. User of our API is usually not interested in implementation details. If he is, then we have documentation :) .

But beware: using mutable because "it's few lines of code less" or something like that is a huge mistake. If you make your method const then keep your word and use mutable with utmost care only! Otherwise you, and users of your code soon will be in big trouble.

Answer 2

This is probably bad practice.

const should normally reflect "logical constness". That is, the logical state of an object shouldn't be modified by a const member function.

The typical case where it makes sense for a const member function to modify some of its object's state is when you're doing some sort of caching.

If, for example, instead of setA you had a getResult that computed a result, then stashed its value in a because 1) it was fairly expensive to compute, and 2) it was fairly likely somebody would get it again soon, then it could/would probably make sense for getResult to compute the value, and push it into the cache.

That obviously modifies the content of the cache, but it does not change the observable behavior of the object (other than the not-so-minor detail of an operation running a lot faster). In short, some bits have changed, but the object's logical state hasn't been affected. For cases like this, you usually want to 1) encapsulate the pointer into something easier to use correctly/harder to abuse, and 2) make that wrapper mutable, to make its role obvious.

Given that it's named setA (and it's public), however, it sounds like something external to the class might be calling it with the expectation of actually changing the object's state. If that's the case, then it's a pretty clear-cut case of poor practice.