1

I'm currently learning C++ and practicing my Knowledge by implementing an simple AddressBook Application.
I started with an Entry class and an AddressBook class which implements a STL Map to access the entries by the last names of the persons.
Now I arrived at the following code:

Entry AddressBook::get_by_last_name(string last_name){
    if(this->addr_map.count(last_name) != 0){
        //What can I do here?
    } else {
        return addr_map[last_name];
    }

In Scripting Languages I would just return something like -1, Error Message(A List in Python) to indicate that the Function failed. I don't want throw an exception, because it's part of the application logic. The Calling Class should be able to react to the request by printing something on the console or opening a Message Box.
Now I thought about implementing the Scripting Languae Approach in C++ by introducing some kind of an Invalid State to the Class Entry. But isn't that bad practice in C++? Could it be that my whole class design is just not appropriate? I appreciate any help. Please keep in mind that I'm still learning C++.

Raidri
  • 17,258
  • 9
  • 62
  • 65

7 Answers7

2

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:

  1. 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.
    1. 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 Entryindicating 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.

eladidan
  • 2,634
  • 2
  • 26
  • 39
  • 1
    Good, but VERY wrong about "every call to get_by_last_name would need to be wrapped with a try...catch block". What are you going to do in the `catch` block - Return an error code? Raise an exception? There's a few cases where you may want to 'silently' do something if the element can't be found, in which case a `lastNameExists` method may be a helpful addition. – Roddy Feb 18 '13 at 14:06
  • See last edit which reflects a more careful study of the try...catch approach – eladidan Feb 18 '13 at 14:21
1

One dead-simple option is to change the return type to Entry* (or const Entry*) and then return either the address of the Entry if found, or NULL if not.

If you use Boost, you could return a boost::optional<Entry>, in which case your success code would be the same, but on not-found you'd say return boost::none. This is fancier, but does about the same thing as using a pointer return type.

John Zwinck
  • 239,568
  • 38
  • 324
  • 436
  • Will boost::optional change the size of my returned Type? I mean it must save that extra information anywhere right? – cpp_hobbyist Feb 18 '13 at 12:12
  • use `nullptr` instead of `NULL` if you are using a compiler that is fairly recent. It is a c++11 keyword for exactly that a pointer that is 0 http://stackoverflow.com/questions/1282295/what-exactly-is-nullptr – Harald Scheirich Feb 18 '13 at 12:33
1

Throwing an exception is definitely the 'correct' C++ thing to do, based on your function return type.

You might want a function like this to help you, though:

bool AddressBook::lastNameExists(const string &last_name)
{
    return addr_map.count(last_name) > 0;
}

Note that your current code returns the entry 'by value' so modifying the returned entry won't update the map. Not sure if this is by accident or design...

Roddy
  • 66,617
  • 42
  • 165
  • 277
  • But now we search the address book twice. Once to test if it is there and once to get the value. – Martin York Feb 18 '13 at 15:14
  • @LokiAstari - agreed, in certain cases, you might. And that's not perfect. My real point is that often, trying to access a map entry which doesn't exist is a program logic error, (rather than maybe a user validation error) . – Roddy Feb 18 '13 at 15:45
1

Other answers have given various approaches, most of them valid. I didn't see this one yet:

You could add a second parameter with a default value:

Entry AddressBook::get_by_last_name(string last_name, const Entry& default_value){
    if(this->addr_map.count(last_name) == 0){
        return default_value; 
    } else {
        return addr_map[last_name];
    }

In this particular instance, there might not be a sensible default value for a non-existing last name, but in many situations there is.

Sjoerd
  • 6,837
  • 31
  • 44
0

In C++ you have several ways of signalling that an issue happened in your function.

You can return a special value which the calling code will recognize as an invalid value. This can be a NULL pointer if the function should return a pointer, or a negative value if your function returns an index in an array, or, in the case of a custom class (e.g. your Entry class) you can define a special Entry::invalid value or something similar that can be detected by the calling function.

Your calling code could look like

if ( entryInstance->get_by_last_name("foobar") != Entry::invalid) 
{
  // here goes the code for the case where the name is valid
} else {
  // here goes the code for the case where the name is invalid
}

On the other hand you can use the C++ exceptions mechanism and make your function throw an exception. For this youcan create your own exception class (or use one defined in the standard library, deriving from std::exception). Your function will throw the exception and your calling code will have to catch it with a try...catch statement.

try
  {
    entryInstance->get_by_last_name("foobar")
  }
  catch (Exception e)
  {
    // here goes the code for the case where the name is invalid
  }
  // here goes the code for the case where the name is valid
Louen
  • 3,617
  • 1
  • 29
  • 49
0

Apart from the fact that you could have more than one entry per surname.

Eliminate the getter, and you've solved the problem, or at least shifted it elsewhere.

Tell the AddressBook to display people with given surnames. If there aren't any it can do nothing.

AddressBookRenderer renderer;
AddressBook contacts;

contacts.renderSurnames("smith", renderer);
contacts.renderCompletions("sm", renderer);
//etc
Peter Wood
  • 23,859
  • 5
  • 60
  • 99
0

You can do what std::map (and the other containers do).

You return an iterator from your search function.
If the search does not find a value that is useful return an iterator to end().

class AddressBook
{
        typedef  <Your Container Type>  Container;
    public:
        typedef  Container::iterator   iterator;


        iterator get_by_last_name(std::string const& lastName) {return addr_map.find[lastName];}

        iterator end()                                         {return addr_map.end();}
};

Your address book is a container like object.
Not finding an item in a search is likely to happen but it does not have enough context to incorporate error handling code (As the address book could be used from lots of places and each place would have different error handling ideas).

So you must move the test for not found state out of your address book.
just like "Python" we return a marker. In C++ this is usually an iterator to end() which the calling code can check and take the appropriate action.

 AddressBook&  ab = getAddressBookRef();
 AddressBook::iterator find = ab.get_by_last_name("cpp_hobbyist");
 if (find != ab.end())
 {
     Entity&  person *find;  // Here you have a reference to your entity.
     // you can now manipulate as you want.
 }
 else
 {
     // Display appropriate error message
 }
Martin York
  • 257,169
  • 86
  • 333
  • 562