What should function returning a reference return on failure?

Discussion in 'C++' started by Jim Langston, May 8, 2006.

  1. Jim Langston

    Jim Langston Guest

    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
     
    Jim Langston, May 8, 2006
    #1
    1. Advertising

  2. Jim Langston

    BigBrian Guest

    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.
     
    BigBrian, May 8, 2006
    #2
    1. Advertising

  3. Jim Langston

    Andre Kostur Guest

    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.
     
    Andre Kostur, May 8, 2006
    #3
  4. * 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?
     
    Alf P. Steinbach, May 8, 2006
    #4
  5. Jim Langston

    Jim Langston Guest

    "Alf P. Steinbach" <> wrote in message
    news:...
    >* 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.
     
    Jim Langston, May 8, 2006
    #5
  6. In article <yUI7g.8$>,
    "Jim Langston" <> 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
     
    Howard Hinnant, May 8, 2006
    #6
  7. Jim Langston

    Phlip Guest

    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, May 8, 2006
    #7
  8. Jim Langston

    Phlip Guest

    > 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!!!
     
    Phlip, May 8, 2006
    #8
  9. Jim Langston

    Jim Langston Guest

    "Alf P. Steinbach" <> wrote in message
    news:...
    >* 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;
     
    Jim Langston, May 8, 2006
    #9
  10. * 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?
     
    Alf P. Steinbach, May 8, 2006
    #10
  11. On Mon, 8 May 2006 08:43:37 -0700, "Jim Langston"
    <> wrote:

    >catch ( int )


    Then don't use an int but use a named (empty) class.
     
    Olaf van der Spek, May 8, 2006
    #11
  12. Jim Langston

    Jim Langston Guest

    "Jim Langston" <> wrote in message
    news:OGJ7g.77$...
    >
    > "Alf P. Steinbach" <> wrote in message
    > news:...
    >>* 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, May 8, 2006
    #12
  13. Jim Langston

    Jim Langston Guest

    "Olaf van der Spek" <> wrote in message
    news:...
    > On Mon, 8 May 2006 08:43:37 -0700, "Jim Langston"
    > <> 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.
     
    Jim Langston, May 8, 2006
    #13
  14. On Mon, 8 May 2006 09:31:28 -0700, "Jim Langston"
    <> wrote:

    >"Olaf van der Spek" <> wrote in message
    >news:...
    >> On Mon, 8 May 2006 08:43:37 -0700, "Jim Langston"
    >> <> 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.
     
    Olaf van der Spek, May 8, 2006
    #14
  15. Jim Langston

    Sean Guest

    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
     
    Sean, May 8, 2006
    #15
  16. Jim Langston

    Alan Johnson Guest

    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
     
    Alan Johnson, May 8, 2006
    #16
  17. Jim Langston

    Phlip Guest

    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!!!
     
    Phlip, May 8, 2006
    #17
  18. * 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?
     
    Alf P. Steinbach, May 8, 2006
    #18
  19. Jim Langston

    Amit Limaye Guest

    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
     
    Amit Limaye, May 9, 2006
    #19
  20. Jim Langston

    Sean Guest

    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::eek: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 (...).
     
    Sean, May 9, 2006
    #20
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Ilias Lazaridis
    Replies:
    0
    Views:
    593
    Ilias Lazaridis
    Feb 1, 2005
  2. Replies:
    11
    Views:
    681
    Christos Georgiou
    May 2, 2006
  3. Replies:
    0
    Views:
    494
  4. _Kevin
    Replies:
    3
    Views:
    80
    _Kevin
    Oct 6, 2006
  5. JustMe
    Replies:
    1
    Views:
    182
    Tassilo v. Parseval
    Aug 29, 2003
Loading...

Share This Page