Returning a map from a class C++

Question

I've written a class that reads in a map. But I need the map to be editable outside of the class. So my question is how can I return a map.

class ReadMap
{
    string fileName;
public:
    //constructors and destructor
    ReadMap(){fileName="blank.txt";}
    ReadMap(string name){fileName=name;}
    ~ReadMap(){}
    //Function to print out visible list
    void show()
    {
        LineDatabase Entry;
        int LineNumber=100;
        string buffer;
        ifstream myfile (fileName.c_str() );
        while (myfile.good())
        {
            myfile >> LineNumber >> ws;
            getline (myfile, buffer);  
            Entry.insert(pair<int, string>(LineNumber, buffer));
            cout <<buffer << endl;
        }
        //return Entry;
    }
};

Not related to your question, but you will insert an item into `Entry` even when `getline` fails. You will observe this bug as "my last item is inserted twice." — Robᵩ, May 19 '11 at 22:45
[Don't test myfile.good().](http://stackoverflow.com/questions/4324441/testing-stream-good-or-stream-eof-reads-last-line-twice/4324667#4324667) — Fred Nurk, May 19 '11 at 22:53

score 4 · Answer 1 · answered May 19 '11 at 22:53

4

You may be better off by having the caller of show() pass in a reference to the map to be filled, as returning a map tends to have high overhead. Something like this:

void show(LineDatabase& Entry) {
    // do your map readin logic
    return;
}

answered May 19 '11 at 22:53

bodes

61
3

score 1 · Answer 2 · answered May 19 '11 at 22:44

1

You return a map just like you return anything else—using the return keyword. If you're wondering why your commented out return doesn't work, it's because you declare show() as returning void. Change void show() to LineDatabase show().

Also, try to keep variable and type names lower-case. Typical convention is to use capitalized names for template parameters, so it is a bit confusing to read.

answered May 19 '11 at 22:44

Cory Nelson

29,236
5
72
110

FWIW, where I come from, type names are InitialCapCamelCase, but variable names are initialSmallCamelCase. – Robᵩ May 19 '11 at 22:47
3

Of course people are free to name things how they wish—I’m just trying to impress upon him the conventions used by C++’s own standard library, and the respected Boost library. Capitalized type names aren’t so bad, but capitalized variable names are pretty unheard of! – Cory Nelson May 19 '11 at 22:52
If he changes the return type and uncommented the return he would be returning the Map by value and therefore using a copy. This is inefficient and possibly dangerous depending on how LineDatabase handles it's copy (shallow vs deep) – cjh May 19 '11 at 23:34
Returning has the most concise semantics. Passing a reference in, you don't know what it's going to do with it. It won't be slow if your compiler implements NRVO (pretty much all modern ones) or rvalue references (only very new ones). In any case, his question wasn't about efficiency, but about how to return a map. – Cory Nelson May 19 '11 at 23:43
I agree with the conciseness of returning, and I am also not in favor of the client passing in a reference. I see no advantage to returning it by value rather than storing it as a field and returning a reference to it as per my answer. – cjh May 20 '11 at 00:00
1

@suicideducky: The advantage, as Cory mentioned, is that the compiler can implement [NRVO](http://en.wikipedia.org/wiki/NRVO). The way you've implemented it, you've made NRVO impossible. So in your attempt to increase efficiency, you've done just the opposite. – Benjamin Lindley May 20 '11 at 00:48
It is true that it makes NVRO impossible, but if you are relying on NVRO you are relying on a compiler optimization (which is non-portable) rather than a language feature which achieves the same thing. – cjh May 20 '11 at 01:27

cjh · Answer 3 · 2011-05-21T06:34:33.317

There are 4 options.

The simplest option is to change the return type of show and uncomment your return, however this will be returning the map by value which will involve a copy and could (depending upon size) be very inefficient (possibly dangerous, depending upon LineDatabase's copy operator).

LineDatabase show()
{
  LineDatabase Entry;
  // .... ommited
  return Entry;
}

The 2nd option is to do as was suggested by user258808 and create a new object then return it by pointer, the issue with this approach is that your client would have to know to call delete on this pointer when finished otherwise you would be creating a leak.

The 3rd option is to have Entry as a field of ReadMap and then return a reference. This is my personal preference as it imposes the least burden on the client, however it may also require you to 'reset' the Entry before each new run. Something like this

class ReadMap
{
    string fileName;
    LineDatabase Entry;
public:
    //constructors and destructor
    ReadMap(){fileName="blank.txt";}
    ReadMap(string name){fileName=name;}
    ~ReadMap(){}
    //Function to print out visible list
    LineDatabase& show()
    {
        int LineNumber=100;
        string buffer;
        ifstream myfile (fileName.c_str() );
        while (myfile.good())
        {
            myfile >> LineNumber >> ws;
            getline (myfile, buffer);  
            Entry.insert(pair<int, string>(LineNumber, buffer));
            cout <<buffer << endl;
        }
        return Entry;
    }
};

The issue with this is that it exposes your internal state to modification, it is possible to return a const reference but then the client cannot modify the Map.

Finally, you could do as was suggested by bodes. However this requires that the client passes in a Map for you to work on.

Your choice will depend on how much work you would like to require your client to do as well as what kind of constraints you need and/or do not need to place on the data structure.

Since the client needs to be able to manipulate the map, there's no reason the client shouldn't create an instance and pass it in by reference. Obviously, the client has to know enough about the map object that they could be trusted to make one. — dusktreader, May 19 '11 at 23:50
That is true, however the client cannot be sure what will happen to the Map he passes in. Is the algorithm destructive? Does it empty the Map before doing it's job? etc. I don't see any advantage to requiring the client to do additional work. — cjh, May 19 '11 at 23:55
-1 for strongly recommending never to return by value. That's how we do things in C++. — Benjamin Lindley, May 20 '11 at 00:15
That too is bad advice and too general. If you need a copy, you pass by value. If you don't need a copy, you pass by const reference. If you need to modify an object, you pass by non-const reference. — Benjamin Lindley, May 20 '11 at 23:24
@Benjamin +1 for summing it up quickly and nicely. I (miss)interpreted your previous statement as pass by value was the ONLY way 'we' do things in C++. — cjh, May 21 '11 at 06:29

Returning a map from a class C++

3 Answers3