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]

Jim Langston wrote:

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.

[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.

--
A: Because it messes up the order in which people normally read text.
Q: Why is it such a bad thing?
A: Top-posting.
Q: What is the most annoying thing on usenet and in e-mail?

[Andre Kostur]

On Mon, 08 May 2006 07:50:00 -0700, Jim Langston wrote:

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.

[Jim Langston]

"Alf P. Steinbach" <al***@start.no> wrote in message
news:4c*************@individual.net...

* 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.

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]

In article <yU***********@fe05.lga>,
"Jim Langston" <ta*******@rocketmail.com> wrote:

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]

Jim Langston wrote:

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
http://c2.com/cgi/wiki?ZeekLand <-- NOT a blog!!!

[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?

--
Phlip
http://c2.com/cgi/wiki?ZeekLand <-- NOT a blog!!!

[Jim Langston]

"Alf P. Steinbach" <al***@start.no> wrote in message
news:4c*************@individual.net...

* 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.

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:

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?

Leave out the 'const'.
--
A: Because it messes up the order in which people normally read text.
Q: Why is it such a bad thing?
A: Top-posting.
Q: What is the most annoying thing on usenet and in e-mail?

[Olaf van der Spek]

On Mon, 8 May 2006 08:43:37 -0700, "Jim Langston"
<ta*******@rocketmail.com> wrote:

catch ( int )

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

[Jim Langston]

"Jim Langston" <ta*******@rocketmail.com> wrote in message
news:OG************@fe05.lga...

"Alf P. Steinbach" <al***@start.no> wrote in message
news:4c*************@individual.net...

* 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.

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]

"Olaf van der Spek" <Ol********@GMail.Com> wrote in message
news:qr********************************@4ax.com...

On Mon, 8 May 2006 08:43:37 -0700, "Jim Langston"
<ta*******@rocketmail.com> wrote:

catch ( int )

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]

On Mon, 8 May 2006 09:31:28 -0700, "Jim Langston"
<ta*******@rocketmail.com> wrote:

"Olaf van der Spek" <Ol********@GMail.Com> wrote in message
news:qr********************************@4ax.com.. .

On Mon, 8 May 2006 08:43:37 -0700, "Jim Langston"
<ta*******@rocketmail.com> wrote:

catch ( int )
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.

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]

Jim Langston wrote:

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).

--
Alan Johnson

[Phlip]

Alf P. Steinbach wrote:

* Phlip:

4) pass an optional sentinel object into FindMap, and return that.
5) return an iterator, and check if that == .end().
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?

Leave out the 'const'.

CMap & found = FindMap(2);

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

--
Phlip
http://www.greencheese.us/ZeekLand <-- NOT a blog!!!

[Alf P. Steinbach]

* Phlip:

Alf P. Steinbach wrote:

* Phlip:

4) pass an optional sentinel object into FindMap, and return that.
5) return an iterator, and check if that == .end().
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?

Leave out the 'const'.

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. ;-)

--
A: Because it messes up the order in which people normally read text.
Q: Why is it such a bad thing?
A: Top-posting.
Q: What is the most annoying thing on usenet and in e-mail?

[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::out_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 (...).

[werasm]

Phlip wrote:

Alf P. Steinbach wrote:

* Phlip:

4) pass an optional sentinel object into FindMap, and return that.
5) return an iterator, and check if that == .end().
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?
Leave out the 'const'.

CMap & found = FindMap(2);

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

Should not work - cannot bind an rvalue (temporary) to a non-constant
reference. I know it compiles under MS VC++ 7.1, but that is erroneous.
This leaves you with this option (or an exception):
....
{
std::map<unsigned int, CMap*>::iterator ThisMapIt =
World.Maps.find( ThisPlayer.Character.Map );

if ( ThisMapIt != World.Maps.end() )
{
return *((*ThisMapIt).second);
}
else
{
//Same base type as CMap, but you know that!
static NullMap sentinal;
return sentinal;
}
}


--
Phlip
http://www.greencheese.us/ZeekLand <-- NOT a blog!!!

[werasm]

Jim Langston wrote:

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. 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!
}
Instead of you map containing CMap*, I would consider using a map of
type:
std::map<unsigned int, boost::shared_ptr<CMap> >

FindMap could then return boost::shared_ptr or boost::weak_ptr. The
advantange of this is that the client can store the pointer if he needs
to. He can't use the pointer if invalid (or at least him using it will
cause visible problems). He could also test for validity of the
returned value (No sentinal required). This he could of course do using
normal pointers too (returning NULL), but who's to say the map doesn't
change after finding the item, or who's to say the client doesn't
decide to store the map entry for some reason. I like weak_ptr in this
case because one doesn't want the item to exist after being removed
from the map which weak_ptr enforces, but shared_ptr does not..

Regards,

W
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