HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Memoization helper

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
helpermemoizationstackoverflow

Problem

Please review:

/// Fetch a value from the `map` or create a new one from the `fun` (memoization).
/// Example: \code
///   flat_map fcache;
///   Frople& frople = getOrElseUpdate (fcache, id, [&]() {return Frople (id);});
/// \endcode
template
inline auto& getOrElseUpdate (M& map, const K& key, F fun) {
  auto it = map.find (key);
  if (it != map.end()) return it->second;
  return map.emplace (std::make_pair (key, fun())) .first->second;
}


Named after the Scala method.

The function is used to simplify the common map operations of checking whether there is a value in a map and generating the value if it isn't in the map, allowing one to use a simple to read one-liner instead of repeating these common but hard-to-read operations in the code.

P.S. Incorporating bits of advise from @vnp and @Loki reviews:

template
inline auto getOrElseUpdate (C& map, typename C::key_type const& key, F funct) ->decltype (map[key]) {
  auto it = map.find (key);
  if (it != map.end()) return it->second;
  return map.emplace (key, funct()) .first->second;
}

Solution

Well overall impression is the code is very condensed. Some white space may help.

I am going to have to disagree with @vnp about the type names.

Its pretty common to use short template type names. Common conventions T generic type C container K key, F functor. If you have a lot then fine you may give more description but the type should be generic. I don't want to impose my thoughts on the user of the type (let the compiler impose its rules on the user). I would not want to limit the use of this to maps if it can generically be applied to other container types. As long as the parameter and variable names are descriptive then the type names don't need to be.

The Key type is unnecessary. This information can be extracted from the container.

Don't assume the internal value is std::pair use the container to tell you its internal value type C::value_type (yes its a std::pair for std::map but don't lock yourself to this type if you don't need to). But since you are using emplace() you should not be passing the internal object (that's relay for insert()). You can just pass the parameters used to construct the internal type.

Don't like the emplace and return of a part of the result in a single line.

template
inline auto getOrElseUpdate(C& associativeCont, typename C::key_type const& key, F funct) -> decltype (associativeCont[key])
{
    auto it = associativeCont.find(key);

    if (it != associativeCont.end())
    {   return it->second;
    }

    // pair
    //    iterator points at inserted value
    //    bool indicates success. Since
    //    we already checked for an existing value it will
    //    always succeed so no need to re-check.
    auto inserted = associativeCont.emplace(key, function());

    // Return the value that was inserted.
    return inserted.first->second;
}

Code Snippets

template<typename C, typename F>
inline auto getOrElseUpdate(C& associativeCont, typename C::key_type const& key, F funct) -> decltype (associativeCont[key])
{
    auto it = associativeCont.find(key);

    if (it != associativeCont.end())
    {   return it->second;
    }

    // pair<iterator, bool>
    //    iterator points at inserted value
    //    bool indicates success. Since
    //    we already checked for an existing value it will
    //    always succeed so no need to re-check.
    auto inserted = associativeCont.emplace(key, function());

    // Return the value that was inserted.
    return inserted.first->second;
}

Context

StackExchange Code Review Q#54500, answer score: 4

Revisions (0)

No revisions yet.