How to iterate over a std::map full of strings in C++

Question

I have the following issue related to iterating over an associative array of strings defined using std::map.

-- snip --
class something 
{
//...
   private:
      std::map<std::string, std::string> table;
//...
}

In the constructor I populate table with pairs of string keys associated to string data. Somewhere else I have a method toString that returns a string object that contains all the keys and associated data contained in the table object(as key=data format).

std::string something::toString() 
{
        std::map<std::string, std::string>::iterator iter;
        std::string* strToReturn = new std::string("");

        for (iter = table.begin(); iter != table.end(); iter++) {
           strToReturn->append(iter->first());
           strToReturn->append('=');
           strToRetunr->append(iter->second());
           //....
        }
       //...
}

When I'm trying to compile I get the following error:

error: "error: no match for call to ‘(std::basic_string<char,
    std::char_traits<char>, std::allocator<char> >) ()’".

Could somebody explain to me what is missing, what I'm doing wrong? I only found some discussion about a similar issue in the case of hash_map where the user has to define a hashing function to be able to use hash_map with std::string objects. Could be something similar also in my case?

Solution 1

Your main problem is that you are calling a method called first() in the iterator. What you are meant to do is use the property called first:

...append(iter->first) rather than ...append(iter->first())

As a matter of style, you shouldn't be using new to create that string.

std::string something::toString() 
{
        std::map<std::string, std::string>::iterator iter;
        std::string strToReturn; //This is no longer on the heap

        for (iter = table.begin(); iter != table.end(); ++iter) {
           strToReturn.append(iter->first); //Not a method call
           strToReturn.append("=");
           strToReturn.append(iter->second);
           //....
           // Make sure you don't modify table here or the iterators will not work as you expect
        }
        //...
        return strToReturn;
}

edit: facildelembrar pointed out (in the comments) that in modern C++ you can now rewrite the loop

for (auto& item: table) {
    ...
}

Solution 2

1. Don't write a toString() method. This is not Java. Implement the stream operator for your class.

2. Prefer using the standard algorithms over writing your own loop. In this situation, std::for_each() provides a nice interface to what you want to do.

3. If you must use a loop, but don't intend to change the data, prefer const_iterator over iterator. That way, if you accidently try and change the values, the compiler will warn you.

Then:

std::ostream& operator<<(std::ostream& str,something const& data)
{
    data.print(str)
    return str;
}

void something::print(std::ostream& str) const
{
    std::for_each(table.begin(),table.end(),PrintData(str));
}

Then when you want to print it, just stream the object:

int main()
{
    something    bob;
    std::cout << bob;
}

If you actually need a string representation of the object, you can then use lexical_cast.

int main()
{
    something    bob;

    std::string  rope = boost::lexical_cast<std::string>(bob);
}

The details that need to be filled in.

class somthing
{
    typedef std::map<std::string,std::string>    DataMap;
    struct PrintData
    {
         PrintData(std::ostream& str): m_str(str) {}
         void operator()(DataMap::value_type const& data) const
         {
             m_str << data.first << "=" << data.second << "\n";
         }
         private:  std::ostream& m_str;
    };
    DataMap    table;
    public:
        void something::print(std::ostream& str);
};

Solution 3

Change your append calls to say

...append(iter->first)

and

... append(iter->second)

Additionally, the line

std::string* strToReturn = new std::string("");

allocates a string on the heap. If you intend to actually return a pointer to this dynamically allocated string, the return should be changed to std::string*.

Alternatively, if you don't want to worry about managing that object on the heap, change the local declaration to

std::string strToReturn("");

and change the 'append' calls to use reference syntax...

strToReturn.append(...)

instead of

strToReturn->append(...)

Be aware that this will construct the string on the stack, then copy it into the return variable. This has performance implications.

Solution 4

iter->first and iter->second are variables, you are attempting to call them as methods.

Solution 5

Note that the result of dereferencing an std::map::iterator is an std::pair. The values of first and second are not functions, they are variables.

Change:

iter->first()

to

iter->first

Ditto with iter->second.

View more solutions

Comments

[omar ba44]

Ah, yes! Now I'm aware of (after you explained) that I'm trying to use first as method :( and yes I shouldn't allocate but this isn't the original code. There I'm allocating the space for string on the heap and returning a reference to it and not copying the object onto the stack. Thank you for the kind answer!

[Martin York]

@ crazybyte: Actually you will be returning a copy of somthing as the return type is std::string. The result is copy constructed aback to the caller. So it sounds like you are leaking memory somwhere.

[Tom Leys]

Using a streaming operator is a good idea. A function with the signature std::ostream& operator<<(std::ostream& stream, std::pair<std::string,std::string>& pair) would allow you to go cout << *itr

[Martin York]

@Tom: No such a good idea as that is not the definition of the map iterator. It may just happens to be what your implementation uses as the map iterator (PS. The fist string (aka KEY) must be constant). Also note that nowhere do I actually explicitly use an iterator.

[Alnitak]

isn't that operator<<() function missing a & on the return type?

[Eric]

When iterating over containers in c++, this is the rare case where i++ vs ++i matters. i++ creates a temp container while ++i does not. I edited your answer, I hope you don't mind

[Jeff Hammerbacher]

You need to change '=' to "="

[Marco Luglio]

in c++11 you can use for(auto iter = table.begin(); iter != table.end(); ++iter) {...}

[Marco Luglio]

Or even a simpler version for(auto& item : table) {...}

[jeroent]

for ( auto iter : table ) { KeyType &key=iter.first; ValueType &value=iter.second; }