CODEWITHSUNDEEP
c++stdstdmapstdlist
Andrea Mugnaini
Andrea Mugnaini

Reputation: 11

Best way to remove an element from a std::list nested inside a std::map

As title says, I would like to know what's the most efficient way to remove an element from a list inside a map that meets certain conditions(name and date). Here's the function I provided:

void Register::DeleteActivity(const Date &f,const std::string &a) {
    auto it = Registro.find(f);
    if(it != Registro.end()) {
        if(it->second.empty()) {
            std::cout <<"Error"<<std::endl;
        } else {
            for(auto ip = it->second.begin(); ip != it->second.end();) {
                if(ip->getName() == a && ip->getStartdate() == f){
                    ip->printInfo();
                
                    it->second.erase(ip);
                } else {
                    ip++;
                }
            }
        }
    } else {
        std::cout<< "DeleteActivity::day not found"<<std::endl;
    }
}

And here is the full class:

class Register {
    private:
        map<Date,std::list<Activity>> Registro;
    public:
        Register(){};
        void addActivity(Date &z, Activity &n);
        void editActivity(const Date &a, const std::string &c, Date k, const std::string newname);
        void DeleteActivity(const Date &f, const std::string &a);

}

Upvotes: 1

Views: 140

Answers (2)

PirklW
PirklW

Reputation: 484

your code has an segmentation fault as erase invalidates the iterator but returns an iterator to the next element. It should be:

ip = it->second.erase(ip);

A pre-increment is also more efficient than post increment, as the original value has not to be stored and the iterator copied:

++ip;

Otherwise, if you do not have duplicates, I'd use std::set instead of std::list, which should as well be more performant (log instead of linear).

#include <iostream>
#include <map>
#include <set>


std::map<const int, std::set<int>> map_set({{0, {1, 2, 3, 4}},
                                            {1, {10, 20, 30, 40}},
                                            {2, {100, 200, 300, 400}},
                                            {3, {}}});


void delete_from_map_set(const int map_index, int value) {

    const auto & map_it = map_set.find(map_index);

    if(map_it != map_set.end()) {

        map_it->second.erase(value);
    }
}


void print_map_set() {
    for(auto & map_it : map_set) {
        std::cout << map_it.first << ": [";
        for(auto & list_it : map_it.second) {
            std::cout << list_it << ", ";
        }
        std::cout << "]" << std::endl;
    }
}


int main(int argc, char * argv[]) {

    print_map_set();

    delete_from_map_set(0, 1);

    print_map_set();

    return 0;
}

Upvotes: 0

molbdnilo
molbdnilo

Reputation: 66459

There's a bug: you need ip = it->second.erase(ip);.
erase invalidates the iterator and returns an iterator to the next element.

But you can get rid of the loop and let the list do the work:

else {
    it->second.remove_if([&f, &a](const Activity& act) 
                                 { return act.getName() == a 
                                       && act.getStartdate() == f; });
}

Upvotes: 2

Related Questions