Is it correct to have a const function which modified a 'scratchpad' variable?

Question

I'll describe the problem. I have a class with API, which invokes a large hierarchy of class member functions to do some logic. Now, I updated the logic so each function in the hierarchy requires an extra argument (the API did not change).

A thought - instead of adding an extra parameter to each method, I can add a 'scrathpad' member to the class, I.e a variable which is only used for temporary calculations, and is only 'valid' inside the time scope of API call and is 'garbage' once the call is finished.

Example:

void A::api()
{
   scratch_pad = get_some_value_once();
   foo1();
}
void A::foo1() { ...; foo2(); }
void A::foo2() { ...; foo3(); }
...
void A::fooN() /* Called 100000000 times */
{ 
     ...; 
     // Do something with scratch_pad.
     // I would realy like to avoid adding 'scratch_pad' parameter to all the foos().
}

Is this a valid approach?

Will it still be valid if my API is declared as const?

WTH is a _"scratchpad"_ variable? Could you illustrate that with some (pseudo) code example please? — πάντα ῥεῖ, May 15 '16 at 12:57
If it doesn't change the obversable status of the object, I think it's fine. — songyuanyao, May 15 '16 at 12:58
_"Now, I updated the logic so each function in the hierarchy requires an extra argument (the API did not change)."_ That sounds like inheritance should be used to replace an interface definition, but it's actually impossible to tell from your unclear context. — πάντα ῥεῖ, May 15 '16 at 13:04
Please note that if you go this way, it will make your class not thread-safe, and adding synchronisation will make it slower. — Revolver_Ocelot, May 15 '16 at 13:11
I don't get this question. If the new value is required from the caller, then it needs to be part of the API. If it's not determined by the caller, then if it's a class invariant, it's a class data member; otherwise it's just a value that you compute from the other parameters and the class state. — Kerrek SB, May 15 '16 at 13:13
@KerrekSB it is not a part of invariant. It is part of externally unobservable state. — Basilevs, May 15 '16 at 13:15
Why not always call `get_some_value_once` and hide the memoization logic inside *that* function? — Kerrek SB, May 15 '16 at 13:32
Are you only *reading* the scratchpad value? Then constness doesn't seem to be a problem. — Kerrek SB, May 15 '16 at 13:33
Are you only reading the scratchpad value? Then constness doesn't seem to be a problem. – Kerrek SB --- But I also write it in the api call... — Elad Weiss, May 15 '16 at 13:38
How about setting the value in the class constructor: `class api { const int value; public: api() : value(get_some_value_once()) {}` I guess that's what you were asking originally. — Kerrek SB, May 15 '16 at 13:44
What I want is not to pass the value throught the entire hierarchy of member functions being invoked by api() - (because I don't want to add a new parameter to all those functions). I wrote it in my question, and in the comment to your solution answer. — Elad Weiss, May 15 '16 at 13:49

jtlim · Answer 1 · 2016-05-16T19:16:35.850

Please don't do this.

You're making your object instance bigger (take more memory) even though you only need variables for the lifespan of the internal function calls.

You're making your functions inherently multithread unsafe, or require locking when there was no need to previously.

You're making the code inherently less obvious/maintainable.

If you want to use const in your API calls, then you need to start adding mutable variables (ugh).

Use a local variable instead. Check Basilevs' answer for one way to do this.

score 2 · Accepted Answer · edited May 15 '16 at 14:02

2

In general, changing a member’s value in a const method is ok when that member is not part of the observable state of the class.

You’ll have to mark your ‘scratchpad’ member with the mutable keyword.
Otherwise, you’ll get a compilation error if you assign to it in a const method.

Check out the isocpp FAQ - https://isocpp.org/wiki/faq/const-correctness#mutable-data-members

edited May 15 '16 at 14:02

πάντα ῥεῖ

1
13
116
190

answered May 15 '16 at 13:22

galsh83

550
3
16

score 1 · Answer 3 · answered May 15 '16 at 13:28

Major problem with class members used as a temporary storage is thread-safety. When methods are invoked from another thread, your member storage for internal data will be reused in an unpredictable way. It also introcudes a hidden coupling between methods.

Effectively, what you need is an additional (probably immutable) state with lifetime of your API call. A usual way to handle states and lifetimes in C++ is to use classes.

I'd recommend extracting new argument to a new immutable class, move required methods to new class and make API implementation create such class and call them.

Instead of:

class API {
   private:
   int state;
   void implement() {
     cout << state << endl;
   }
   public:
   void execute() {
      state = 1;
      implement();
   }
}

Do:

class State {
   int value;
   public:
   State(int valueArg): value(valueArg) {}
   void implement() const {
     cout << state << endl;
   }

}

class API {
   public:
   void execute() const {
      State(1).implement();
   }
}

This is much better than using the proposed 'scratchpad' variable approach. I'd recommend making State a private inner class of API though. — jtlim, May 16 '16 at 19:10

Is it correct to have a const function which modified a 'scratchpad' variable?

3 Answers3