views:

505

answers:

13

Let us say we have a function of the form:

const SomeObject& SomeScope::ReturnOurObject()
{
    if( ! SomeCondition )
    {
        // return early
        return  ;
    }

    return ourObject;
}

Clearly the code above has an issue, if the condition fails then we have a problem as to how to return from this function. The crux of my question is what is the best way to handle such a situation?

+5  A: 

If you didn't want to change the return type to a pointer, you could possibly use the null object pattern

Yacoby
+5  A: 

What's the lifetime of 'ourObject' and where is it created?

If you need to return early without touching/updating ourObject and it exists at the point where you're returning, I don't see a problem with returning a reference to it.

If you're trying to communicate that there has been an error, you'll probably have to throw an exception instead of returning early because the contract your function agreed to says that it will return a reference to 'SomeObject'.

If you're returning a reference to a temporary, you better fix that issue...

Timo Geusch
+1 for warning about temporaries. However the object is a member of the class SomeScope so this isn't a problem. Although the object will exist, in some modes it is inappropriate to access it (hence the protective condition).
Konrad
OK, just trying to cover all the bases. I assumed that ourObject probably was a member. In this case, if you can't access the object and you can't change the function signature to signal its inaccessibility by other means I think you're stuck with throwing an exception unless you can create a second SomeObject instance that basically says "I'm invalid" and return a reference to that.
Timo Geusch
Hmm, that is an interesting thought. Unfortunately the object in question is an STL container so that is not possible.
Konrad
Would it be possible to have an empty container floating around for these purposes? That would probably signal the user that there isn't anything for them to do with the data. Just a thought...
Timo Geusch
+3  A: 

Uh... just throw an exception...

Tyson
Seems a bit heavy handed doesn't it? Not to mention dangerous as every call would have to be wrapped in a try block.
Konrad
I usually wouldn't throw an exception except something unexpected has happened. There are sometimes other good scenarios where it makes sense to throw, but this seems like it isn't one of them.
Adrian Grigore
Why heavy handed? And why would things have to be wrapped in a try block? If a function's contract says that it *will* return a reference to an object and it can't then - almost by definition - that is an exceptional condition. A calling function is free to deal with that exception but it doesn't have to. It could just let the exception propagate out to a layer that can handle it.
Charles Bailey
Indeed, if you need to wrap every call in a try block, exceptions aren't the way to go. But you should point that out in your question. The question doesn't look like that `SomeCondition` being false is an expected condition.
Johannes Schaub - litb
agreed that it's heavy-handed, but when the return type is a reference and can't be changed, you really only have two choices: (1) employ the null object pattern as suggested by Yacoby above or (2) throw an exception. In any case, if the return type is at all malleable, I'd certainly explore changing it to a pointer and spend some time justifying why it's a reference.
Chris Cleeland
+3  A: 

Throw an exception

denisenkom
-1: I usually wouldn't throw an exception except something unexpected has happened. There are sometimes other good scenarios where it makes sense to throw, but this seems like it isn't one of them.
Adrian Grigore
No this is an example where exception is the best choice (beside changing type of return value, or returning some default value). Functions which return reference must return valid reference or throw an exception.
denisenkom
Adrian: if throw an exception is not the best choice then the function should not return a reference. Have it return something else or pass a non-const reference in to the function to be modified.
jmucchiello
+13  A: 

I'd use a pointer instead of a reference in this case. In fact this criterium (optional or mandatory return value) is how I decide between pointers and references in the first place.

Adrian Grigore
I agree with this.
mizipzor
Very good answer! +1
sbi
+1  A: 

I'll consider returning by reference only if I am sure that the function will never fail (Probably like returning a const-reference to an internal member variable). If there are multiple validations inside the function then I will consider one of the following options:

  • If the copying the object is not costly, return a copy
  • If it is costly to copy, return a const-pointer. Return NULL in case of failure.
Naveen
+5  A: 

I would return a boolean and pass the object reference as parameter to the function:

bool getObject(SomeObject& object)
{
    if( condition )
         return false;

    object = ourObject;
    return true;
}

In that case you now your object was filled only if the function return true.

Patrice Bernassola
Interesting, I was thinking along those lines myself.
Konrad
I like this answer. The flag tells you if the passed object was modified.
jmucchiello
This doesn't do the same as the original code, though. The original code returns a reference to ourObject (and so future modifications to ourObject, via non-const references, will be visible via the caller's reference). This makes a copy of ourObject, as it is at the time when the function is called. Neither is necessarily right or wrong, but they have different purposes.
Steve Jessop
+25  A: 

This isn't a syntactic issue, but a design issue. You have to specify what ReturnOurObject() is supposed to return when SomeCondition is true. That depends mainly on what the function is going to be used for. And that you haven't told us.

Depending on the design issues, I see a few possible syntactic ways out of this:

  • return a reference to some other object; you would have to have some ersatz object somewhere
  • have a special "no-object-to-return" object somewhere that you return a reference to; clients can check for this; if they don't check they get reasonable default behavior
  • return a pointer, not a reference; clients would have to always check the return value of the function
  • throw an exception; if SomeCondition is something exceptional which clients can not deal with that would be appropriate
  • assert; if SomeCondition should always hold, it should be asserted
sbi
When the C++ returns references, it's usually implicit references to contained objects, or to the this pointer (self) - simply because of this problem. When returning references to objects, you have to know that you've constructed a valid object. References are sort of useless otherwise.
Chris Kaminski
+1  A: 

If you don't want to throw an exception, you could try something like:

const SomeObject& GetSomeObject(const SomeObject& default_if_not_found = SomeObject())
{
    // ...
    if (!found)
        return default_if_not_found;

    return ourObject;
}

This means you don't have to have some kind of static variable lying around somewhere to return, and you can still call it with no parameters. I guess you'd assume the default-constructed SomeObject() state is the empty state so you can test SomeObject to see if it is in the empty state. It's also sometimes useful to use this to override the default return value, eg. GetSomeObject(SomeObject("I'm empty"))

AshleysBrain
What's the lifetime of a temporary used as a default argument?
Steve Jessop
Temporaries bound to const-references have the same lifetime as the const reference they are bound to. I guess if the temporary is then returned, it gains the lifetime of the const reference receiving the return of the function - which is convenient.
AshleysBrain
A: 

I believe the answer depends on whether you are the kind of programmer who considers exceptions as bad and never wants to use them or not. If you do use them, AND this really is an exceptional condition, then an exception must be thrown. Yes, users need to deal with it, but hiding an error to pretend that nothing has happened is simply not an option. But if you never use exceptions, then just return a pointer, or a boolean, and add a non-const reference to SomeObject to your function. Both solutions are simple, but who said that over-complicating things makes code better?

+7  A: 

I'd probably do this:

const SomeObject& SomeScope::ReturnOurObject()
{
    if( ! SomeCondition )
    {
        throw SomeException();
    }
    return ourObject;
}

const SomeObject *SomeScope::ReturnOurObjectIfPermitted()
{
    return SomeCondition ? &ourObject : 0;
}

And perhaps also:

bool SomeScope::CheckMode();
    return SomeCondition;
}

Callers then have some options:

// 1 - "I know that I'm in the right mode"
myScope.ReturnOurObject().DoSomething();

// 2 - "I don't know whether I'm in the right mode, but I can cope either way"
if (SomeObject *myObject = myScope.ReturnOurObjectIfPermitted()) {
    myObject->DoSomething();
} else {
    DoSomethingElse();
}

// 2 - alternate version:
if (myScope.CheckMode()) {
    SomeObject &myObject = myScope.ReturnOurObject();
    myObject.DoSomething();
} else {
    DoSomethingElse();
}

// 3 - "I don't know whether I'm in the right mode. If I'm not then
// I can't deal with it now, but some higher-level code can"
try {
    // ... several calls deep ...
    myScope.ReturnOurObject().DoSomething();
    // ... several returns back ...
} catch (SomeException &e) {
    DoSomethingElse();
}

You design a function's interface differently according to what the preconditions are, i.e. whose responsibility it is to ensure it can do its job.

If it's the caller's responsibility, then they need the ability to ensure that. You might want to check anyway, just to be on the safe side. Throwing an exception when the so-called "preconditions" aren't met makes them more like advice than conditions, and it can work quite well provided you don't mind the runtime overhead of checks everywhere. I'm usually fairly unforgiving with preconditions, so actually I might replace the conditional exception with an assert, and document that the function must not be called in the wrong mode. It depends how much control the caller has over the mode - obviously if it changes arbitrarily (e.g., if "SomeCondition" is "there is not a byte available on the UART") then you need a very different interface from if it only ever changes when the client code calls some function to change it.

If it's not the caller's responsibility to get the mode right, then your interface shouldn't write cheques that your implementation can't cash. In this case if it's "expected" that there will be no object to return, then the return should not be by reference. Unless either (a) you can rustle up a special "sorry, it didn't work" object to return, that the caller can check for, or (b) you're comfortable throwing exceptions in "expected" situations. IMO (b) is rare in C++. (a) usually isn't any better than returning a null pointer, but occasionally it is. For instance returning an empty collection to mean "there's nothing you're allowed to see here" might result in some very clean calling code, if DoSomethingElse() would have been "do nothing". So as well as the desired responsibilities, the interface depends on the expected use cases.

Steve Jessop
Yes, good solution and well explained.
Konrad Rudolph
+1 for a good explaination and taking the time to provide clear examples.
Konrad
A: 

The way the function is set up, you have to return a SomeObject or throw an exception. If you want to do something else, you need to change the function. There are no other choices.

If SomeCondition will never be false, then you can remove the test in the code and put an assert in instead, or you can keep the test and throw an exception. (I'm loath to recommend just disregarding the possibility, since "never false" almost always means "never false unless something bad has happened and not been detected", and if it's worth testing for under any conditions it's worth detecting.)

There's the possibility of returning some sort of null object. Pointers are useful for that, since there's an obvious null value that tests false, but you could pass a flag back as an out parameter or return a special object intended as a null. Then you're going to need to test the return value on each call for this to be meaningful, and that's not likely to be less hassle than the assert or throw.

If I were doing this, it would depend on SomeCondition. If it has side effects or isn't difficult to calculate, test and throw an exception if false. If it's onerous to calculate, use assert so you don't have to do it in production, and forget about it besides that.

David Thornley
+1  A: 

Exactly three choices...

1.   Throw an exception
2.   Return a dummy error object
3.   Use a pointer and return NULL

DigitalRoss