What should function returning a reference return on failure?

[Jim Langston]

I'm sure this has been asked a few times, but I'm still not sure.

I want to create a function to simplify getting a reference to a CMap in a
map.

This is what I do now in code:

std::map<unsigned int, CMap*>::iterator ThisMapIt = World.Maps.find(
ThisPlayer.Character.Map );
if ( ThisMapIt != World.Maps.end() )
{
CMap& ThisMap = *((*ThisMapIt).second);
// Work with ThisMap
}

Now, the map number should always be in the map, but I'm a bit pedantic and
like to check for all possible errors. I'd like to create a function to do
this like:

CMap& FindMap( const unsigned int MapNumber )
{
std::map<unsigned int, CMap*>::iterator ThisMapIt = World.Maps.find(
ThisPlayer.Character.Map );
if ( ThisMapIt != World.Maps.end() )
return *((*ThisMapIt).second);
else
// What to return here? A reference can't be null!
}

My alternative is to return a CMap* and return NULL on failure, but I would
rather deal with references so I can use . instead of ->

Any suggestions?

I guess I could return a CMap* and in code do:

CMap* MapP = FindMap( ThisPlayer.Character.Map );
if ( MapP != NULL )
{
CMap& ThisMap = *MapP;
// Work with ThisMap
}

if I really have to

 

[BigBrian]

I'm sure this has been asked a few times, but I'm still not sure.

I want to create a function to simplify getting a reference to a CMap in a
map.

This is what I do now in code:

std::map<unsigned int, CMap*>::iterator ThisMapIt = World.Maps.find(
ThisPlayer.Character.Map );
if ( ThisMapIt != World.Maps.end() )
{
CMap& ThisMap = *((*ThisMapIt).second);
// Work with ThisMap
}

Now, the map number should always be in the map, but I'm a bit pedantic and
like to check for all possible errors. I'd like to create a function to do
this like:

CMap& FindMap( const unsigned int MapNumber )
{
std::map<unsigned int, CMap*>::iterator ThisMapIt = World.Maps.find(
ThisPlayer.Character.Map );
if ( ThisMapIt != World.Maps.end() )
return *((*ThisMapIt).second);
else
// What to return here? A reference can't be null!
}

My alternative is to return a CMap* and return NULL on failure, but I would
rather deal with references so I can use . instead of ->

Any suggestions?

Throw an exception.

 

[Andre Kostur]

I'm sure this has been asked a few times, but I'm still not sure.

I want to create a function to simplify getting a reference to a CMap in a
map.

This is what I do now in code:

std::map<unsigned int, CMap*>::iterator ThisMapIt = World.Maps.find(
ThisPlayer.Character.Map );
if ( ThisMapIt != World.Maps.end() )
{
CMap& ThisMap = *((*ThisMapIt).second); // Work with ThisMap
}

Now, the map number should always be in the map, but I'm a bit pedantic
and like to check for all possible errors. I'd like to create a
function to do this like:

CMap& FindMap( const unsigned int MapNumber ) {
std::map<unsigned int, CMap*>::iterator ThisMapIt = World.Maps.find(
ThisPlayer.Character.Map );
if ( ThisMapIt != World.Maps.end() )
return *((*ThisMapIt).second);
else
// What to return here? A reference can't be null!
}

My alternative is to return a CMap* and return NULL on failure, but I
would rather deal with references so I can use . instead of ->

Any suggestions?

I guess I could return a CMap* and in code do:

CMap* MapP = FindMap( ThisPlayer.Character.Map ); if ( MapP != NULL ) {
CMap& ThisMap = *MapP;
// Work with ThisMap
}

if I really have to

That's one of your choices. There are three ways that I can think of:

1) Return a pointer and check for NULL.
2) Throw an exception and catch it somewhere
3) Return a reference to a "NULL CMap", that is a special instance of your
CMap object representing that no map exists.

 

[Alf P. Steinbach]

* Jim Langston:

I'm sure this has been asked a few times, but I'm still not sure.

If the function returns a reference, it's guaranteeing that if it
returns, the result is a valid reference.

You have the choice of using (1) a reference to some special object
denoting "no object" or "failure", or (2) throwing an exception.

(2) is most clean, most reliable wrt. client code checking, and may help
avoid constructing a large dummy object.

 

[Jim Langston]

* Jim Langston:

If the function returns a reference, it's guaranteeing that if it returns,
the result is a valid reference.

You have the choice of using (1) a reference to some special object
denoting "no object" or "failure", or (2) throwing an exception.

(2) is most clean, most reliable wrt. client code checking, and may help
avoid constructing a large dummy object.

I generally only like throwing exceptions on genuine errors, but you know
what? Not finding the CMap in the map is a genuine error, because it should
be there.

Thanks! I'll do that.

 

[Howard Hinnant]

Now, the map number should always be in the map, but I'm a bit pedantic and
like to check for all possible errors. I'd like to create a function to do
this like:

CMap& FindMap( const unsigned int MapNumber )
{
std::map<unsigned int, CMap*>::iterator ThisMapIt = World.Maps.find(
ThisPlayer.Character.Map );
if ( ThisMapIt != World.Maps.end() )
return *((*ThisMapIt).second);
else
// What to return here? A reference can't be null!
}

My alternative is to return a CMap* and return NULL on failure, but I would
rather deal with references so I can use . instead of ->

Any suggestions?

One school of thought is that if the missing map represents a run time
failure that you're anticipating (lack of memory, full disk, bad user
input, etc.) then an exception is appropriate. However if the missing
map represents a programming error, an assert may be more appropriate.

Of course for programs that must keep running no matter what, an assert
is rarely appropriate.

If you throw an exception, will you know what to do with it when you
catch it? If you can correct the error on catch, then an exception
sounds like the way to go. If on catch you're just going to say "I've
got a bug" and terminate, then an assert might actually help you find
the error earlier because it will be reporting it earlier (before stack
unwinding).

-Howard

 

[Phlip]

I generally only like throwing exceptions on genuine errors, but you know
what? Not finding the CMap in the map is a genuine error, because it
should be there.

If it's a _programmer_ error, put assert(false) at the bottom of the
function.

If it's an unpreventable _user_ error, throw. (So also throw if that
assert(false) gets optimized away. And pick a better argument than false.)

To finish Andre's list:

4) pass an optional sentinel object into FindMap, and return that.

If the caller passes no sentinel, construct the NullObject one and return
it:

CMap& FindMap(
const unsigned int MapNumber,
CMap const & sentinel = NullMap() );

 

[Phlip]

4) pass an optional sentinel object into FindMap, and return that.

If the caller passes no sentinel, construct the NullObject one and return
it:

CMap& FindMap(
const unsigned int MapNumber,
CMap const & sentinel = NullMap() );

Oh, my. That's not const-correct. What's the fix?

 

[Jim Langston]

* Jim Langston:

If the function returns a reference, it's guaranteeing that if it returns,
the result is a valid reference.

You have the choice of using (1) a reference to some special object
denoting "no object" or "failure", or (2) throwing an exception.

(2) is most clean, most reliable wrt. client code checking, and may help
avoid constructing a large dummy object.

Dang, there's one problem with the try...catch.

try
{
CMap& ThisMap = FindMap( MapNumber );
}
catch ( int )
{
LogError("Could not find map");
}

That ThisMap is only going to exist during the lifetime of the try block.
And I can't create it outside the block because it's a reference and has to
be initialized.

Now this means I'll have to put whole blocks of code inside the try block,
but I don't want to catch errors in a block for all the code, and a lot of
the code should execute anyway even if they can't find the map.

That is, this code won't compile:

int i = 1;
try
{
int& j = i;
}
catch (...)
{
}

std::cout << j << std::endl;

 

[Alf P. Steinbach]

* Phlip:

Oh, my. That's not const-correct. What's the fix?

Leave out the 'const'.

 

[Olaf van der Spek]

catch ( int )

Then don't use an int but use a named (empty) class.

 

[Jim Langston]

Dang, there's one problem with the try...catch.

try
{
CMap& ThisMap = FindMap( MapNumber );
}
catch ( int )
{
LogError("Could not find map");
}

That ThisMap is only going to exist during the lifetime of the try block.
And I can't create it outside the block because it's a reference and has
to be initialized.

Now this means I'll have to put whole blocks of code inside the try block,
but I don't want to catch errors in a block for all the code, and a lot of
the code should execute anyway even if they can't find the map.

That is, this code won't compile:

int i = 1;
try
{
int& j = i;
}
catch (...)
{
}

std::cout << j << std::endl;

I finally settled on this (ugly) code:

// Get a reference in the map for this player
try
{
CPlayer& ThisPlayer = FindPlayer( Socket );
}
catch ( int )
{
SendMessageToPlayer( Socket, MSG_SERVER_MESSAGE, "<Message not sent,
please relog - Error has been logged>" );
MessageBuffer.push_back( LogError( "Unlogged in client attempting to send
message on socket " + StrmConvert<std::string>( Socket ) + " on IP " +
IP ) );
return 0;
}
CPlayer& ThisPlayer = FindPlayer( Socket );

My feeling being that if it doesn't throw on the first call to FindPlayer
it's not going to throw on the second call to FindPlayer and I can proceed.
If it does throw on the second call somehow then it's an actual program
error and the program can halt on an uncaught error.

 

[Jim Langston]

Then don't use an int but use a named (empty) class.

Umm.. I don't understand what type I decide to throw has to do with the
problem I stated. I threw an int just so I wouldn't have to create an empty
class just to throw. But that wouldn't solve my problem of the the block.

 

[Olaf van der Spek]

Umm.. I don't understand what type I decide to throw has to do with the
problem I stated. I threw an int just so I wouldn't have to create an empty
class just to throw. But that wouldn't solve my problem of the the block.

I should've quoted more:

Now this means I'll have to put whole blocks of code inside the try block,
but I don't want to catch errors in a block for all the code, and a lot of

But that indeed doesn't help you if there's other code that has to
execute even if the key can't be found.

 

[Sean]

Instead of:

try
{
CMap& ThisMap = FindMap( MapNumber );
}

catch ( int )
{
LogError("Could not find map");

}

do:

CMap* tryme;
try
{
tryme = &(FindMap( MapNumber );
CMap& ThisMap = *tryme;
}
catch ( int )
{
LogError("Could not find map");

}

that way you won't have to worry about your scoping issue, and you can
still deal with a reference instead of a pointer when you do your work

 

[Alan Johnson]

Dang, there's one problem with the try...catch.

try
{
CMap& ThisMap = FindMap( MapNumber );
}
catch ( int )
{
LogError("Could not find map");
}

That ThisMap is only going to exist during the lifetime of the try block.
And I can't create it outside the block because it's a reference and has to
be initialized.

Don't try to use exceptions as return codes, See 17.12 and 17.13 from
the FAQ for some suggestions about the proper ways to use exceptions.

Now this means I'll have to put whole blocks of code inside the try block,
but I don't want to catch errors in a block for all the code, and a lot of
the code should execute anyway even if they can't find the map.

You won't be catching errors for all the code. You will only catch
exceptions for which you have a corresponding catch
block. Consider the following:

// Always inherit from std::exception to make your life easier.
#include <stdexcept>
class KeyNotFound : public std::exception
{
// Various members that might make diagnosing the error easier.
};

try
{
CMap& ThisMap = FindMap( MapNumber );
ThisMap.DoSomething();
}
catch (KeyNotFound &e)
{
// Handle this error.
}


Here, if FindMap throws a KeyNotFound exception, it will be caught and
handled appropriately. If any other exception is thrown, from
"ThisMap.DoSomething();" for example, then it is not caught (not by this
code, anyhow).

 

[Phlip]

* Phlip:

5) return an iterator, and check if that == .end().

Leave out the 'const'.

CMap & found = FindMap(2);

Now found refers to the temporary NullMap(), which destructed somewhere
around ;.

 

[Alf P. Steinbach]

* Phlip:

5) return an iterator, and check if that == .end().


CMap & found = FindMap(2);

Now found refers to the temporary NullMap(), which destructed somewhere
around ;.

I assumed NullMap returned a reference to a static CMap. ;-)

 

[Amit Limaye]

Passing the sentinel objects is the best choice i would say but then i
m a C programmer
functions should return status not actual values is the general
principle i program with. so my functions return error codes and the
parameters return actual values

 

[Sean]

I think that in C++ it is better to program with exceptions rather than
error codes as a general principle. I would start with exceptions and
move to error codes and special return values only if there are special
design considerations for the particular application that make them a
better choice.

Also, if you don't want to make a special class to throw your
exceptions, pick one from the standard library. For the particular
case, std:ut_of_range makes the most sense to me. If you always use
exceptions from the standard library (and derive your own from those as
well), users of your code know they will always be able to catch any
exception from you with a "catch (std::exception& e)", which gives them
more information than the basic catch all "catch (...).