1
\$\begingroup\$

In the codebase I support, I found the following code. I am interested in your opinion about its expediency and especially the expediency of redefining the operator().

  template <typename KEY, typename VALUE>
  class MyUnorderedMap {
    std::unordered_map<KEY, VALUE> map;

  public:
    template <typename T>
    auto insert(const T value) {
      return map.insert(value);
    }
    inline auto& operator()(const KEY& r) { return (*this)[r]; }
    inline auto& operator[](const KEY r) { return map[r]; }
    inline auto find(const KEY& key) const { return map.find(key); }
    inline auto size() const { return map.size(); }
    inline auto begin() { return map.begin(); }
    inline auto end() { return map.end(); }

    inline bool exists(const KEY& userId) const {
      return map.find(userId) != map.end();
    }
    bool remove(const KEY& userId) {
      auto it = map.find(userId);
      bool retval = (it != map.end());
      if (retval)
        map.erase(it);
      return retval;
    }
  };
\$\endgroup\$
  • 3
    \$\begingroup\$ Welcome to Code Review! It sounds like this is code you found rather than wrote yourself. Is that the case? If so I'm afraid this is off-topic because we only review code written by the user who posted the review question. \$\endgroup\$ – Null Mar 25 at 15:48
  • \$\begingroup\$ @Null this is kind of crazy, ok, this is my code. more precisely the code that I support \$\endgroup\$ – Max Mar 26 at 10:27
  • 2
    \$\begingroup\$ Perfectly valid thing to review. \$\endgroup\$ – Martin York Mar 27 at 20:04
4
\$\begingroup\$

I question the value of this template class. We hide many important members of std::unordered_map (including crbegin(), empty(), reserve(), at(), try_emplace()) for no good reason. The additional functions that are provided have no business being members: they are perfectly implementable as free functions without privileged access.

Considering the members in turn:

  • insert(): only instantiable if T is convertible to the map's value_type. Unnecessarily copies the argument.
  • operator(): unnecessary, given that we have operator[].
  • operator[]: unnecessarily copies its argument.
  • find(), size() begin(), end(): required only because the map is private.
  • exists(): should be a free function. If this is generic, why is the parameter called userId?
  • remove() - why not just use the erase() that accepts a key? Or do we have to build with pre-C++11 library?

When I've wanted to provide something like the exists() here, I've made it a standalone template function that can work with many kinds of container:

/*
 * contains()
 * A convenience overload that searches from beginning to end of a container.
 *
 * If an iterator is supplied, it will be written with the found position or
 * container.end() as appropriate.
 */
template<typename T, typename U>
inline bool contains(const T& container, const U& value)
{
    using std::begin;
    using std::end;
    return find(begin(container), end(container), value) != end;
}

template<typename T, typename U, typename Iter>
    /* Iter should be a T::iterator or T::const_iterator */
inline bool contains(T& container, const U& value, Iter& it)
{
    using std::begin;
    using std::end;
    return (it = find(begin(container), end(container), value)) != end;
}
/*
 * Overloads for pointer-to-const values within containers of pointer-to-mutable.
 */
template<typename T, typename U>
inline bool contains(const T& container, const U* value)
{
    using std::begin;
    using std::end;
    return find(begin(container), end(container), value) != end;
}

template<typename T, typename U, typename Iter>
inline bool contains(const T& container, const U* value, Iter& it)
{
    using std::begin;
    using std::end;
    return (it = find(begin(container), end(container), value)) != end;
}
/*
 * Overloads for maps (by key)
 */
template<typename T>
inline bool contains(const T& container, const typename T::key_type& key)
{
    return container.find(key) != container.end();
}

template<typename T, typename Iter>
inline bool contains(T& container, const typename T::key_type& key, Iter& it)
{
    return (it = container.find(key)) != container.end();
}

template<typename T>
inline bool contains(const T& container, const typename T::key_type& key,
                     typename T::mapped_type& result)
{
    typename T::const_iterator it;
    return contains(container, key, it) && (result = it->second, true);
}
\$\endgroup\$

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.