5
\$\begingroup\$

Please review:

/// Fetch a value from the `map` or create a new one from the `fun` (memoization).
/// Example: \code
///   flat_map<string, Frople> fcache;
///   Frople& frople = getOrElseUpdate (fcache, id, [&]() {return Frople (id);});
/// \endcode
template<typename M, typename K, typename F>
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<typename C, typename F>
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;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Related: codereview.stackexchange.com/q/35180/15094 \$\endgroup\$ – Morwenn Jun 17 '14 at 13:10
  • \$\begingroup\$ Welcome to Code Review! Your question is missing a lot of details to be clear. Could you add information about how is it used, what it's supposed to be used for and have a bigger picture. \$\endgroup\$ – Marc-Andre Jun 17 '14 at 13:26
  • \$\begingroup\$ @Marc-Andre: Thanks. Added the explanation. If that much isn't obvious from the documentation, though, then perhaps the documentation should be improved. Have any pointers? \$\endgroup\$ – ArtemGr Jun 17 '14 at 13:42
  • \$\begingroup\$ It's not much on the method, but more for your question. It's hard to make a review on such a small piece of code without a proper context. Is is part of a class, are there other methods, is it use often, etc. The more information you give, the better your question will be. \$\endgroup\$ – Marc-Andre Jun 17 '14 at 15:13
  • 2
    \$\begingroup\$ @ArtemGr: When tagging something C++11 it is also worth adding the tag C++ it helps things organize better. \$\endgroup\$ – Martin York Jun 17 '14 at 15:51
4
\$\begingroup\$

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<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;
}
\$\endgroup\$
  • \$\begingroup\$ Short template names are OK. Template types reduced to typename are not. Sorry if I wasn't clear. \$\endgroup\$ – vnp Jun 18 '14 at 9:00
3
\$\begingroup\$

Template parameters are not informative. Better do something like

#define Maptype typename
#define Keytype typename
#define Function typename

template <Maptype M, Keytype K, Function F>

I was seriously confused with fun. Better call it function.


The code failed to compile on my system (GCC 4.7.3). First, auto function was missing a trailing return type; fixed by adding

-> decltype(map[key])

and changing auto& to just auto. I don't know C++11 well enough to blame your compiler (which compiles your code) or mine (which doesn't).

Then it happened so that GCC 4.7.3 does not have emplace. Known GCC problem.

\$\endgroup\$
  • \$\begingroup\$ Thanks. I compile with gcc 4.8.3 and --std=c++1y, sorry for the confusion. \$\endgroup\$ – ArtemGr Jun 17 '14 at 19:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.