Deleting elements from vector inside unordered_map

Question

In my class, I have an unordered map of vectors, like this:

std::unordered_map<State, std::vector<std::shared_ptr<const City>>> citiesByState;

My class also has these two methods:

  void addCity(State state, const std::shared_ptr<const City>& city);
  void removeCity(State state, const std::shared_ptr<const City>& city);

I add a city, like this:

void Manager::addCity(State state, const std::shared_ptr<const City>& city) {
  auto location = citiesByState.find(state); // Find the state in the map
  if (location == citiesByState.end()) { // If the state isn't in the map
    std::vector<std::shared_ptr<const City>> cities; // Create a vector
    cities.push_back(city); // Add the city
    citiesByState[state] = cities; // Add the state and city vector to my map
  } else {
    auto vector = location->second; // Get the city vector. If the city isn't there already, add it.
    if (std::find(vector.begin(), vector.end(), city) == vector.end()) {
      vector.push_back(city);
    }
  }
}

Now here's my code to remove a city:

void Manager::removeCity(State state, const std::shared_ptr<const City>& city) {
  auto location = citiesByState.find(state);
  if (location != citiesByState.end()) {
    auto vector = location->second;
    if (vector.size() > 0) {
      std::cout << "Vector isn't empty." << std::endl;
    }
    vector.clear(); // Just empty it out for now.
  }
}

Then I run it like this:

  City city = ... // get city
  manager->addCity(State::NewYork, city);
  manager->removeCity(State::NewYork, city);

I can call manager->removeCity(State::NewYork, city) repeatedly, and each time, I see the vector is not empty. It seems like I can't remove from the Vector.

What am I doing wrong?

Answer 1

TL;DR

You are removing elements from a copy of the vector, not from the std::vector that exists in the found location of the std::unordered_map.

Long story

When you call auto vector = location->second; in Manager::removeCity you're making a copy of that vector inside the scope of the if statement. Therefore, your changes won't be reflected in the container you're targeting. Only your copy will be affected, and that too goes out of scope by the end of the if statement, so everything that happens if you found a location will not be saved in the state of the std::unordered_map container.

You could fix this by just calling location->second.clear() directly or, if you really want to give it another name, use a reference, e.g. auto& vec = location->second; vec.clear();. Note that this also applies for the Manager::addCity method.

P.S. I would stay away from using variable names that are the same as those of containers or well-established classes in the STL in order to avoid confusion.