STL map beginner

[koopey]

i need to build a simple program to add name and email address using STL map. adding new entry and listing all entries work fine but removing and changing entries dont work.
in code i have declared a map globally. know its bad practice but just a beginner and wanted to focus on map only for now, not pointer.

  ///function to remove entry
map<string, string> remove_entry() ///function to remove an email contact. doesn't work

{
    string name;
    cout<<"Enter name to remove: ";
    cin>>name;

    map<string, string>::iterator itr=name_and_email.begin();
    map<string, string>::iterator itr_end=name_and_email.end();

    for(itr;itr->first!=name&&itr!=itr_end;++itr)
    {
        if(itr->first==name)
        {
            name_and_email.erase(name);
            cout<<"entry erased!";
            break;
        }
    }
    /*if(itr==itr_end)
    {
        cout<<name<<" doesn't exist!";
    }*/
    return name_and_email;
}

///function to change entry

map<string, string> change_entry()///function to modify email address of certain name. crashes program.

{
    string name;
    string email;
    cout<<"Enter name to change: ";
    for(map<string, string>::iterator itr=name_and_email.begin(), itr_end=name_and_email.end();itr->first!=name&&itr!=itr_end;++itr)
    {
        if(itr->first==name)
        {
            cout<<endl<<"current info\n";
            cout<<itr->first<<"----->"<<itr->second<<endl;
            cout<<"Enter the new email: ";
            cin>>email;
            itr->second=email;
            cout<<"Info updated!";
            break;
        }
    }
    return name_and_email;
}

just for the big picture, here is the entire code.
http://cpp.sh/45if
any help would be appreciated.

[Peter87]

In your remove_entry() the inside of the if statement will never run because when itr->first==name becomes true the loop condition makes the loop end before the if statement is reached.

The point of having a map is that you can lookup elements by key efficently without having to loop through each element. The way you are using the map it would be better if you had used a simple vector because that would avoid the overhead of map.

Instead of using a loop, you should have a look at the find function.

http://www.cplusplus.com/reference/map/map/find/

You pass a key to find and it will return an iterator to the element that is associated with that key. If the key is not found it returns an iterator to the end() of the map. So all you have to do is calling find, check if the iterator returned is not an end iterator, and then call erase to remove the element.

When you call erase it's better that you pass in the iterator to the erase function. The iterator already knows the position in the map so removing the element is very efficient. If you pass the key value the map will have to search for the element in the map structure which is just unnecessary work if you already have the iterator.

http://www.cplusplus.com/reference/map/map/erase/

[coder777]

In remove_entry(): Appart from the fact that you cannot enter names with whitespaces it should work. The loop/if is unnecessary. If you want to know whether or not a name exists use the find(...) function of the map.

In change_entry(): itr->first!=name makes your program crash if there's no entry in your map. You can remove it because the if on line 38 does exactly the same. Again: Use the find(...) function to find an existing entry. Plus: you have no cin>>name;

Why do you return the map?

[koopey]

woooot. The code works very well now. thanks guys.

@coder777 when code wasn't working initially, was just trying different things out. hence the return although kind of knew that wouldn't make a difference. but still kind of conused with return statement.
is return statement in above code a bad thing or doesn't matter?

i think that if i declare map locally and pass the local map into function, then return statement would be necessary, if I return the value of function to the same local map using "=" to update the local map.pretty verbose? eg:

int main()
{
    map<string, string> local_map;///declaring local map
    local_map=remove_entry();///  = used to update the value of local map 
}
 

would that be correct?

[coder777]

is return statement in above code a bad thing or doesn't matter?

Without assignment it actually does nothing.

would that be correct?

Nope, you need to actually pass the object. In this case as a reference would be most appropriate:

void remove_entry(map<string, string> &local_map)
{
    string name;
    cout<<"Enter name to remove: ";
    cin>>name;

    /*if(local_map.find(name)==local_map.end())
    {
        cout<<name<<" doesn't exist!";
    }*/

    local_map.erase(name);
    cout<<"entry erased!";
}

int main()
{
    map<string, string> local_map;///declaring local map
// ...
// Add name
// ...
    remove_entry(local_map);

    return 0;
}

[koopey]

you need to actually pass the object

sorry that's what i meant. just forgot to show in code.
what i meant was, instead of using pointers and references, can we use use return and "=" to update the local variable. in above eg:

void remove_entry(map<string, string> &local_map)
{
//... the same stuff as above
cout<<"entry erased!";
return local_map;///use of return
}
int main()
{
    map<string, string> local_map;///declaring local map
// ...
// Add name
// ...
    local_map=remove_entry(local_map);///this is the core of my question. using local map= if ok

if it is acceptable, then,
1. how does this work internally?remove_entry(local_map);
when remove_entry is called, is a duplicate variable of local_map created by copying our original "local_map", making this process slower than pointers and references(as you did above)?