Search code examples
c++stlapi-designstdmapc++98

How to hide iterators in the API to consume an item in a std::map wrapper


In a c++98 project I have a class Items that wraps a std::map:

#include <string>
#include <map>

class Item { /* ... */ }; // Some class holding some data

class Items
{
 public:
    typedef std::map<std::string,Item> container_type;
    typedef container_type::iterator iterator;

    Items() {}

    iterator find(const String& k) { return i_map.find(k); }
    iterator end() { return i_map.end(); }
    void erase(iterator i) { i_map.erase(i); }
    // ...

 private:
    container_type i_map;
};

Its main use is to search a certain Item and if found use and remove it. The first obvious API I proposed to consume an item was this:

Items items;
Items::iterator i = items.find("some-id");
if( i!=items.end() )
   {
    const Item& item = i->second; // Get and use the item
    items.erase(i); // Item consumed: remove it from items
   }

...But I was asked to hide the concepts of iterator and pair from the class API. To meet this new requirement the first idea was to store internally an iterator i_found to remember the last found item:

#include <stdexcept>
#include <string>
#include <map>

class Item { /* ... */ }; // Some class holding some data

class Items
{
 public:
    typedef std::map<std::string,Item> container_type;
    typedef container_type::iterator iterator;

    Items() : i_found( i_map.end() ) {}

    #define should_be_const // const // Unfortunately 'i_found' must be a non const 'iterator' in order to be erased

    bool contains(const std::string& k) should_be_const
       {
        i_found = i_map.find(k);
        return i_found!=i_map.end();
       }

    const Item& get(const std::string& k) should_be_const
       {
        if(i_found==i_map.end() || k!=i_found->first) i_found = i_map.find(k); // search if necessary
        if(i_found!=i_map.end()) return i_found->second;
        else throw std::runtime_error("key \'" + std::string(k) + "\' not found!");
       }

    void erase_found()
       {
        i_map.erase(i_found);
        i_found = i_map.end(); // invalidate last find
       }

 private:
    container_type i_map;
    mutable iterator i_found; // Last found item
};

This gives the possibility to write:

Items items;
if( items.contains("some-id") )
   {
    const Item& item = items.get("some-id"); // Get and use the item
    items.erase_found(); // Item used: remove it from items
   }

I know that if this is an improvement or not is debatable, I'm not asking about that (yeah, I don't like it either). Is there a way in this last implementation to make the methods contains() and get() const?

Given the requirement above, I was also interested in suggestions about different approaches. While Item copy construction is acceptable, I'd like to to avoid to construct an item if "some-id" was not found, as in this alternative I was desperately trying:

bool Items::extract_if_present(const std::string& k, Item& item)
   {
    iterator i = i_map.find(k);
    if( i != i_map.end() )
       {
        item = i->second;
        i_map.erase(i);
        return true;
       }
    return false;
   }

Item item; // <-- Avoidable useless work if not found
if( items.extract_if_present("some-id", item) )
   {
    //item; // Use the copied item
   }

Solution

  • With optional(not std, which is C++17, but could be done in C++98 (using boost for example)), then you might have in interface

    optional<Item> get(const std::string&) const;
    optional<Item> extract(const std::string&);
    

    With usage similar to:

    Items items;
    // ...
    if (optional<Item> item = items.extract("some-id"))
    {
        // use *item;
    }