-2

I have a program that reads lines from a file looking like this:

...
name1 (123)
name2 (345)
...

It then stores each line as an object of MyClass in a map called namelist. The key of namelist is the name of the object the value is the object itself. In the first version of the code namelist was of type map< string,MyClass >, and the objects were created in the loop as MyClass obj; (and ofc all '->' were '.'. But that only created a map of equal objects, why? I've read that you rarely need to use 'new' on objects in c++. Why do i need it here?

ifstream myfile ("input.txt");
map<string,MyClass*> namelist;
string line;

while ( getline(myfile,line) ) {

    istringstream iss(line);
    string word;

    while (iss>>word) {
        MyClass* obj = new MyClass;
        obj->name = word;
        iss>>word;
        sscanf(word.c_str(),"(%d)",&obj->number);
        namelist.insert( pair<string,MyClass*>(obj->name,obj) );
    }
}
myfile.close();
  • 3
    [You don't need it!](https://stackoverflow.com/questions/46991224/are-there-any-valid-use-cases-to-use-new-and-delete-raw-pointers-or-c-style-arr) – user0042 Dec 10 '17 at 17:57
  • 1
    You don't need to use `new`. The common use for `new` is linked lists. Try without `new`. – Thomas Matthews Dec 10 '17 at 17:57
  • I don't see anything in that code that requires `new`. Why not just use `std::map`? – Galik Dec 10 '17 at 18:00
  • I expect the reason is the OP tried to take the address of the local variable and put that in the map. It won't work that way since the local variable goes out of scope ... – drescherjm Dec 10 '17 at 18:04
  • 1
    OT: sscanf? Why don’t you read it into the number? –  Dec 10 '17 at 18:20
  • @manni66 Because the numbers are contained in brackets, which i am not interested in.This is a way of being certain that i don't accidentally try to read the brackets into the number. – Gustaf Holst Dec 10 '17 at 20:57

2 Answers2

1

You don't need new. Just do map<string,MyClass> namelist and MyClass obj; (default constructor w/ stack allocation)

new is for allocating objects on the heap and the data must be explicitly deallocated with delete. Instead, simply make your map hold MyClass objects instead of pointers to such objects. The map object will copy the object onto the heap and take care of deallocation for you. .

Del
  • 1,309
  • 8
  • 21
  • 1
    Using this will also help you prevent accidental memory leaks, as the deallocation is handled by std::map and not by the programmer. – Aryan Dec 10 '17 at 18:05
0

You don't need new as std::map already takes care of dynamic allocation.

std::map<std::string, MyClass> namelist;

You don't even need to create a temporary variable first and insert that into the map, which creates an unneccessary copy. Instead, creating the object directly within the map will be more efficient.

The following technique requires MyClass to have a default constructor only. Apart from being more efficient, this technique also enables you to use classes that do not have a copy constructor (e. g. std::ifstream) as map values.

To get the same semantics as std::map::insert() an explicit call to find() is required. If you can live with the fact that duplicate keys overwrite existing objects in the map, you can simplify the code by removing the if.

// Check if the key does not exist yet.                 
if (namelist.find(word) == namelist.end()){
    // Default-construct object within map and get a reference to it.
    MyClass& obj = namelist[word]; 
    // Do something with inserted obj...
    obj.name = word;
}

If your class has a constructor with one or more parameters, that you would like to call during in-place construction, you need a C++11 feature, namely std::map::emplace().

// Construct MyClass within the map, if the key does not exist yet.                 
// Pass arguments arg1, arg2, argN to the constructor of MyClass.
auto res = namelist.emplace(arg1, arg2, argN);
if (res.second){ // new object inserted?
    // Get a reference to the inserted object.
    MyClass& obj = *res.first;
    // Do something with inserted obj...
    obj.name = word;
}
zett42
  • 25,437
  • 3
  • 35
  • 72
  • First of all i would like to say that i think both answers were worthy of the "best answer" tag, however this one was more elaborate. Second thank you for not only answering the question but explaining why. I guess the way i was printing *namelist* out was misleading me to think that the same object was added over and over. I cant reproduce the behaviour. Third thanks for the tip on creating the object directly within the map. I still have problems wrapping my head around that way of using references. Very educational, thanks! – Gustaf Holst Dec 10 '17 at 20:48