the hard way
-
Recently, i was reworking some code. The library was used to read in sets of rules, and process other data based on those rules. Over time, this library had evolved such that there were two different methods of processing rules ("simple" rules required a different code path than "complex" rules), and four different file formats for the rule storage. So before working on the required changes, i first put some work into reducing (so far as possible) the redundant code. As part of this, i wrote a class to parse the various file formats and present their contents in a consistent manner:
class CRuleReader
{
public:
CRuleReader();void Open(...);
bool NextRule();
...long GetId() const;
LPCTSTR GetCondition() const;
LPCTSTR GetValue() const;
...
};This worked quite well, except for one thing: it was very slow. The methods for accessing rule data needed a fair bit of time behind the scenes to parse out the required information, validate it, and present it in a consistent manner. Fortunately, there was a simple solution: cache the results of internal calculations, and re-use the cached results whenever possible. I quickly made this change, and was quite pleased with the results... However, since these
constmethods now needed to write to the internal cache data, I'd done something rather ugly in these methods:const_cast<CRuleReader*>(this)->CacheValueId(LPCTSTR key, long id);
Unpleasant as it looked, it allowed me to keep the public face of the class clean - methods that logically modified state were non-
const, methods that did not wereconst. All things considered, i was still reasonably happy with it. Then, a few days later, i stumbled on some similar code, and at last realized, that in over a decade of using C++, i'd managed to either avoid or forget themutablekeyword... :sigh: :doh: :-O----
It appears that everybody is under the impression that I approve of the documentation. You probably also blame Ken Burns for supporting slavery.
--Raymond Chen on MSDN
-
Recently, i was reworking some code. The library was used to read in sets of rules, and process other data based on those rules. Over time, this library had evolved such that there were two different methods of processing rules ("simple" rules required a different code path than "complex" rules), and four different file formats for the rule storage. So before working on the required changes, i first put some work into reducing (so far as possible) the redundant code. As part of this, i wrote a class to parse the various file formats and present their contents in a consistent manner:
class CRuleReader
{
public:
CRuleReader();void Open(...);
bool NextRule();
...long GetId() const;
LPCTSTR GetCondition() const;
LPCTSTR GetValue() const;
...
};This worked quite well, except for one thing: it was very slow. The methods for accessing rule data needed a fair bit of time behind the scenes to parse out the required information, validate it, and present it in a consistent manner. Fortunately, there was a simple solution: cache the results of internal calculations, and re-use the cached results whenever possible. I quickly made this change, and was quite pleased with the results... However, since these
constmethods now needed to write to the internal cache data, I'd done something rather ugly in these methods:const_cast<CRuleReader*>(this)->CacheValueId(LPCTSTR key, long id);
Unpleasant as it looked, it allowed me to keep the public face of the class clean - methods that logically modified state were non-
const, methods that did not wereconst. All things considered, i was still reasonably happy with it. Then, a few days later, i stumbled on some similar code, and at last realized, that in over a decade of using C++, i'd managed to either avoid or forget themutablekeyword... :sigh: :doh: :-O----
It appears that everybody is under the impression that I approve of the documentation. You probably also blame Ken Burns for supporting slavery.
--Raymond Chen on MSDN
My experience says, that after you write lines like:
const_cast<CRuleReader*>(this)->CacheValueId(LPCTSTR key, long id);
maintaining programmers sometimes stick needles into a doll with your face :-)
------------------------- Don't worry, be happy :o)
-
Recently, i was reworking some code. The library was used to read in sets of rules, and process other data based on those rules. Over time, this library had evolved such that there were two different methods of processing rules ("simple" rules required a different code path than "complex" rules), and four different file formats for the rule storage. So before working on the required changes, i first put some work into reducing (so far as possible) the redundant code. As part of this, i wrote a class to parse the various file formats and present their contents in a consistent manner:
class CRuleReader
{
public:
CRuleReader();void Open(...);
bool NextRule();
...long GetId() const;
LPCTSTR GetCondition() const;
LPCTSTR GetValue() const;
...
};This worked quite well, except for one thing: it was very slow. The methods for accessing rule data needed a fair bit of time behind the scenes to parse out the required information, validate it, and present it in a consistent manner. Fortunately, there was a simple solution: cache the results of internal calculations, and re-use the cached results whenever possible. I quickly made this change, and was quite pleased with the results... However, since these
constmethods now needed to write to the internal cache data, I'd done something rather ugly in these methods:const_cast<CRuleReader*>(this)->CacheValueId(LPCTSTR key, long id);
Unpleasant as it looked, it allowed me to keep the public face of the class clean - methods that logically modified state were non-
const, methods that did not wereconst. All things considered, i was still reasonably happy with it. Then, a few days later, i stumbled on some similar code, and at last realized, that in over a decade of using C++, i'd managed to either avoid or forget themutablekeyword... :sigh: :doh: :-O----
It appears that everybody is under the impression that I approve of the documentation. You probably also blame Ken Burns for supporting slavery.
--Raymond Chen on MSDN
Then, a few days later, i stumbled on some similar code, and at last realized, that in over a decade of using C++, i'd managed to either avoid or forget the mutable keyword... If it makes you feel any better, I've done the same! Thanks for telling me about mutable
Regards - Roger
-
My experience says, that after you write lines like:
const_cast<CRuleReader*>(this)->CacheValueId(LPCTSTR key, long id);
maintaining programmers sometimes stick needles into a doll with your face :-)
------------------------- Don't worry, be happy :o)
-
Heh. :) To be fair, some of that mess is a copy-paste error. :-O
----
...the wind blows over it and it is gone, and its place remembers it no more...
Anyway, complex casts, attempts to gain write access from const methods - and everything that introduces side effects to a program - are a perfect method to make your customer hate you...
------------------------- Don't worry, be happy :o)
-
Anyway, complex casts, attempts to gain write access from const methods - and everything that introduces side effects to a program - are a perfect method to make your customer hate you...
------------------------- Don't worry, be happy :o)
Well, in this case, the side-effects are hidden: logically, the
constmethods actconst, as they never modify data that can be seen from the outside. A nicer solution would have been to solve the performance issues by re-writing other components, but that would have greatly expanded the scope of this change. Another potential strategy would have been to pre-cache everything, but that would have improved the (unlikely) worst-case scenarios at the expense of the much more common scenarios. That said, i still cringe whenever i see aconst_cast(or C-cast used as aconst_cast), and am very happy i was able to remove them.----
...the wind blows over it and it is gone, and its place remembers it no more...
-
Recently, i was reworking some code. The library was used to read in sets of rules, and process other data based on those rules. Over time, this library had evolved such that there were two different methods of processing rules ("simple" rules required a different code path than "complex" rules), and four different file formats for the rule storage. So before working on the required changes, i first put some work into reducing (so far as possible) the redundant code. As part of this, i wrote a class to parse the various file formats and present their contents in a consistent manner:
class CRuleReader
{
public:
CRuleReader();void Open(...);
bool NextRule();
...long GetId() const;
LPCTSTR GetCondition() const;
LPCTSTR GetValue() const;
...
};This worked quite well, except for one thing: it was very slow. The methods for accessing rule data needed a fair bit of time behind the scenes to parse out the required information, validate it, and present it in a consistent manner. Fortunately, there was a simple solution: cache the results of internal calculations, and re-use the cached results whenever possible. I quickly made this change, and was quite pleased with the results... However, since these
constmethods now needed to write to the internal cache data, I'd done something rather ugly in these methods:const_cast<CRuleReader*>(this)->CacheValueId(LPCTSTR key, long id);
Unpleasant as it looked, it allowed me to keep the public face of the class clean - methods that logically modified state were non-
const, methods that did not wereconst. All things considered, i was still reasonably happy with it. Then, a few days later, i stumbled on some similar code, and at last realized, that in over a decade of using C++, i'd managed to either avoid or forget themutablekeyword... :sigh: :doh: :-O----
It appears that everybody is under the impression that I approve of the documentation. You probably also blame Ken Burns for supporting slavery.
--Raymond Chen on MSDN
Shog9 wrote:
that in over a decade of using C++, i'd managed to either avoid or forget the mutable keyword...
Whoa. Me too, until I just read your post. I had a very similar problem recently. I had a static instance of a derived class that ensured thread-safety via a
boost::mutex. Unfortunately, if the derived class would attempt to lock this mutex, I couldn't make the bloody function doing itconstso I used a nasty hack similar to yours. I will now go back and look atmutableforthwith. There is a good overview of this on the C++ FAQ: http://www.parashift.com/c++-faq-lite/const-correctness.html#faq-18.13[^] -
Well, in this case, the side-effects are hidden: logically, the
constmethods actconst, as they never modify data that can be seen from the outside. A nicer solution would have been to solve the performance issues by re-writing other components, but that would have greatly expanded the scope of this change. Another potential strategy would have been to pre-cache everything, but that would have improved the (unlikely) worst-case scenarios at the expense of the much more common scenarios. That said, i still cringe whenever i see aconst_cast(or C-cast used as aconst_cast), and am very happy i was able to remove them.----
...the wind blows over it and it is gone, and its place remembers it no more...
I remember having such a problem some time ago. I made it this way:
class Something
{
...public:
// Affects only the following call(s) of Calculate() method. // void PreCacheSomething(); // Does the actual job. // bool Calculate() const;};
In the code:
someObject.PreCacheSomething();
someObject.Calculate();If you mean it - say it.
------------------------- Don't worry, be happy :o)
-
I remember having such a problem some time ago. I made it this way:
class Something
{
...public:
// Affects only the following call(s) of Calculate() method. // void PreCacheSomething(); // Does the actual job. // bool Calculate() const;};
In the code:
someObject.PreCacheSomething();
someObject.Calculate();If you mean it - say it.
------------------------- Don't worry, be happy :o)
Dmitry Khudorozhkov wrote:
someObject.PreCacheSomething(); someObject.Calculate();
Now you've changed the interface though. The caller needs to be aware of what, really, is an implementation detail. Not to mention, it just relocates the problem - the caller now needs a non-
constreference tosomeObject, even though logically they only needconstmethods.----
...the wind blows over it and it is gone, and its place remembers it no more...
-
Dmitry Khudorozhkov wrote:
someObject.PreCacheSomething(); someObject.Calculate();
Now you've changed the interface though. The caller needs to be aware of what, really, is an implementation detail. Not to mention, it just relocates the problem - the caller now needs a non-
constreference tosomeObject, even though logically they only needconstmethods.----
...the wind blows over it and it is gone, and its place remembers it no more...
I just thought... What about using another object for precaching, and passing this object to someObject?
void Calculate(CacheObject* cache) const
------------------------- Don't worry, be happy :o)
-
I just thought... What about using another object for precaching, and passing this object to someObject?
void Calculate(CacheObject* cache) const
------------------------- Don't worry, be happy :o)
That would work, and does address the second point i made (although it still complicates the interface). I often do something similar in ASP.NET apps, where using the app's cache object solves a lot of problems and allows me to keep caching policy for items together with the code that generates those items. Another way would be to use global or static class data, though this brings with it a different set of problems and limitations.
----
...the wind blows over it and it is gone, and its place remembers it no more...
-
That would work, and does address the second point i made (although it still complicates the interface). I often do something similar in ASP.NET apps, where using the app's cache object solves a lot of problems and allows me to keep caching policy for items together with the code that generates those items. Another way would be to use global or static class data, though this brings with it a different set of problems and limitations.
----
...the wind blows over it and it is gone, and its place remembers it no more...
Interface complication is the least of evils here, I think.
Shog9 wrote:
global
That's the evil!
Shog9 wrote:
static class data
I'd say no, unless class is a singleton.
------------------------- Don't worry, be happy :o)
-
Interface complication is the least of evils here, I think.
Shog9 wrote:
global
That's the evil!
Shog9 wrote:
static class data
I'd say no, unless class is a singleton.
------------------------- Don't worry, be happy :o)
Dmitry Khudorozhkov wrote:
I'd say no, unless class is a singleton.
As would i, unless:
- The class will never be used across threads (or is able to accept responsibility for synchronization). Or,
- The data will be initialized once, explicitly, and never actually modified by any instance methods (this is a variation on the "pre-cache everything" strategy, and comes in handy when i'm going to be creating / destroying lots of these objects.)
----
...the wind blows over it and it is gone, and its place remembers it no more...
-
Shog9 wrote:
that in over a decade of using C++, i'd managed to either avoid or forget the mutable keyword...
Whoa. Me too, until I just read your post. I had a very similar problem recently. I had a static instance of a derived class that ensured thread-safety via a
boost::mutex. Unfortunately, if the derived class would attempt to lock this mutex, I couldn't make the bloody function doing itconstso I used a nasty hack similar to yours. I will now go back and look atmutableforthwith. There is a good overview of this on the C++ FAQ: http://www.parashift.com/c++-faq-lite/const-correctness.html#faq-18.13[^]Rob Caldecott wrote:
There is a good overview of this on the C++ FAQ: http://www.parashift.com/c++-faq-lite/const-correctness.html#faq-18.13\[^\]
Heh... sure 'nuff. :)
----
...the wind blows over it and it is gone, and its place remembers it no more...