Some quick notes about your code:
if(this->addr_map.count(last_name) != 0){
//What can I do here?
You probably wanted it the other way:
if(this->addr_map.count(last_name) == 0){
//handle error
But your real problem lies here:
return addr_map[last_name];
Two things to note here:
- The
operator[]
for map can do 2 things: If the element exists, it returns it; If the element doesn't exist, it creaets a new (key,value) pair
with the specified key and value's default constructor
. Probably not what you wanted. However, if your if
statement from before would have been the right way, then the latter would never happen because we would knowthe key exists before hand.
- In calling
count()
before, you effectively tell map
to try and find the element. By calling operator[]
, you are telling map
to find it again. So, you're doing twice the work to retrieve a single value.
A better (faster) way to do this involves iterators, and the find
method:
YourMap::iterator it = addr_map.find(last_name); //find the element (once)
if (it == addr_map.end()) //element not found
{
//handle error
}
return *it.second; //return element
Now, back to the problem at hand. What to do if last_name
is not found?
As other answers noted:
- Simplest solution would be to return a pointer (NULL if not found)
- Use
boost::optional
.
- Simply return the
YourMap::iterator
but it seems that you are trying to "hide" the map
from the user of AddressBook
so that's probably a bad idea.
throw
an exception
. But wait, now you'll have to first check that calling this method is 'safe' (or handle the exception
when appropriate). This check requires a boolean method like lastNameExists
which would have to be called before calling get_by_last_name
. Of course then we'er back to square 1. We're performing 2 find operations to retrieve a single value. It's safe, but if you're doing A LOT of calls to get_by_last_name
then this is potentially a good place to optimize with a different solution (besides, arguably the exception is not very constructive: What's wrong with searching for something that isn't there, huh?).
- Create a
dummy
member for Entry
indicating that is not a real Entry
but that is very poor design (unmanageable, counter intuitive, wasteful - you name it).
As you can see, the first 2 solutions are by far preferable.