Hello newsgroup readers,

I would like to know if the caching strategy used in code below is a
good idea. The code explains my question... (It is a simplified version
of what I'm currently working on.)

class X
{
	// Datastructure Action
	struct Action {
		enum Type {
			Type_A,
			Type_B,
			Type_C // etc...
		};
		int mID;
		Type mType;
		std::string mDescription;
	};

	// Collection of actions
	std::vector< Action > mActions; // contains many (say 500+) actions

	// Extract all ActionIDs for a given ActionType
	const std::vector< int >& GetActionIDs( Action::Type inActionType )
	{
		// Use caching
		static std::map< Action::Type, std::vector< int > > fCache;

		// First search cache
		std::map< Action::Type, std::vector< int > >::iterator theIt =
fCache.find( inActionType );

		// If ActionType not found in cache then iterate all Actions
		if( theIt == fCache.end() )
		{
			std::vector< int > theActionIDs;
			for( int theIndex = 0; theIndex != mActions.size(); ++theIndex )
			{
				if( mActions[ theIndex ].mType == inActionType )
				{
					theActionIDs.push_back( mActions[ theIndex ].mID );
				}
			}
			// Store result in cache
			fCache[ inActionType ] = theActionIDs;

			// Return result from cache ( so we can return a reference)
			return fCache[ inActionType ];
		}
		// Else immediately return the result from the cache
		return theIt->second;
	}
};

I have recently been writing code like this. Do you think it's a good
practice?


Greetings,
Francis


      [ See http://www.gotw.ca/resources/clcm.htm for info about ]
      [ comp.lang.c++.moderated.    First time posters: Do this! ]

francis_r wrote:
> I would like to know if the caching strategy used in code below is a
> good idea. The code explains my question... (It is a simplified
> version of what I'm currently working on.)
>
> class X
> {
> // Datastructure Action
> struct Action {
> enum Type {
> Type_A,
> Type_B,
> Type_C // etc...
> };
> int mID;
> Type mType;
> std::string mDescription;
> };
>
> // Collection of actions
> std::vector< Action > mActions; // contains many (say 500+) actions
>
> // Extract all ActionIDs for a given ActionType
> const std::vector< int >& GetActionIDs( Action::Type inActionType )
> {
> // Use caching
> static std::map< Action::Type, std::vector< int > > fCache;
>
> // First search cache
> std::map< Action::Type, std::vector< int > >::iterator theIt =
> fCache.find( inActionType );
>
> // If ActionType not found in cache then iterate all Actions
> if( theIt == fCache.end() )
> {
> std::vector< int > theActionIDs;
> for( int theIndex = 0; theIndex != mActions.size(); ++theIndex )
> {
> if( mActions[ theIndex ].mType == inActionType )
> {
> theActionIDs.push_back( mActions[ theIndex ].mID );
> }
> }
> // Store result in cache
> fCache[ inActionType ] = theActionIDs;

This statement doesn't check if 'theActionIDs' is empty and will
create an empty entry in the cache.  Is that acceptable?

> // Return result from cache ( so we can return a reference)
> return fCache[ inActionType ];
> }
> // Else immediately return the result from the cache
> return theIt->second;
> }
> };
>
> I have recently been writing code like this. Do you think it's a good
> practice?

It seems incomplete.  How do you clear the cache and force it to be
re-collected if the contents of 'mActions' change?

If for some reason 'mActions' is a "grow only" container, your scheme
can work, but it still seems (a) underdesigned and (b) wasteful because
it stores copies of vectors in the cache instead of, say, pointers to
the vectors.

V
-- 
Please remove capital 'A's when replying by e-mail
I do not respond to top-posted replies, please don't ask



      [ See http://www.gotw.ca/resources/clcm.htm for info about ]
      [ comp.lang.c++.moderated.    First time posters: Do this! ]

> > // Store result in cache
> > fCache[ inActionType ] = theActionIDs;
>
> This statement doesn't check if 'theActionIDs' is empty and will
> create an empty entry in the cache.  Is that acceptable?

Yes, I see no reason why not. If there are no actions for a certain
ActionID then the code will return the empty vector from the dict. This
seems logical to me.

> > // Return result from cache ( so we can return a reference)
> > return fCache[ inActionType ];
> > }
> > // Else immediately return the result from the cache
> > return theIt->second;
> > }
> > };
> >
> > I have recently been writing code like this. Do you think it's a good
> > practice?
>
> It seems incomplete.  How do you clear the cache and force it to be
> re-collected if the contents of 'mActions' change?

That's a valid concern. But in my case the mActions never changes at
runtime, so that won't pose a problem.
I also forgot to mention that the class X is a singleton. If it wasn't
then the same cache would be shared among all objects. That would
probably not be desirable ;)

> If for some reason 'mActions' is a "grow only" container, your scheme
> can work, but it still seems (a) underdesigned and (b) wasteful because
> it stores copies of vectors in the cache instead of, say, pointers to
> the vectors.

The copy waste can indeed be reduced by using pointers.

Thanks for your feedback.

Francis


      [ See http://www.gotw.ca/resources/clcm.htm for info about ]
      [ comp.lang.c++.moderated.    First time posters: Do this! ]