Fog Creek Software
Discussion Board




Multiple return points in VC++

Hi folks,

Well, I've been coding for some 3 years now in VC++. This question has been bugging me for long. I got different answers from different people, so I've decided to post it over here now.

I have 2 functions below. SomeFunc is getting a SAFEARRAY inside a variant.

HRESULT SomeClass::SomeFunc( VARIANT* pvarSafe )
{
    if( pvarSafe != NULL && pvarSafe->parray != NULL )
     {
         // Do something
     }
    
     return S_OK;
}

HRESULT SomeClass::SomeFunc( VARIANT*pvarSafe )
{
    if( pvarSafe == NULL )
        return E_FAIL;

    if( pvarSafe->parray == NULL )
        return E_FAIL;

    // Do something
    
     return S_OK;
}

If I use the only return from 1 place, the code has too many nested ifs. Some people prefer it while some advice against it. Which is better & should be used. Is there any reason for avoiding multiple returns???

Very very confused
Monday, September 01, 2003

A rule often stated for "good" function design is:
Use only one return statement.
Another rule often stated is: Check your variables on entry, do it in an explicit and unambiguous manner and bail out as soon as possible if anything is wrong.
This will keep the rest of the code free from checks that clutter up the "real" code and thus make you function easy to read and thus easy to maintain. This will also gain you some inherent robustness as errors are more easily spotted.
On the other hand you don't want lot's of exit points in your function as they will make it harder to read and understand.

Geert-Jan Thomas
Monday, September 01, 2003

lot's = lots

Geert-Jan Thomas
Monday, September 01, 2003

The two functions you posted are nnot equivalent, so I'm not surprised people tell you to do it differently. Personally I would do something like:

{
  HRESULT hr = E_FAIL;

  // if (params ok)
  {
    // do something


    hr = S_OK;
  }

  return hr;
}

That's how I was taught to do it because it is supposedly easier to follow by looking at the code. I imagine that you would get very similar optimised code from both you original multiple-exit function as my code above, however. If you have to handle exceptions from other code in your function it is somewhat easier my way, I think - you just make sure that the hr = S_OK is the last thing inside the try{} block and hr will be correctly set regardless of whether you handle an exception or not (which is why I always set the return value pessimistically at the start of the function).


Monday, September 01, 2003

http://discuss.fogcreek.com/joelonsoftware/default.asp?cmd=show&ixPost=31001&ixReplies=34

Earlier thread regarding single/multiple exit points.

Patrik
Monday, September 01, 2003

Personally I dislike the habit of returning constants directly such as return S_OK;

The constant is only correct if the surrounding code is correct, and you typed the right constant in.

I prefer a local variable to have the current status of the function, that status is initialised as failure (whatever failure means in the context).

The status is changed only when it is positively a success and in most circumstances there's only one place in the function that happens.

All returns from the function return the status which is guaranteed to be a failure unless it is the successful fork.

Now this can be written as a single exit function, but most commonly has at least two exits and if its a very long set of serial tests it might consist of returns on every proven failure.  But treat all functions as their own state machine and the chances of error are reduced.

Simon Lucy
Monday, September 01, 2003

>>Personally I dislike the habit of returning constants directly such as return S_OK;

But if you are creating COM objects you don't have a choice.

Tony E
Monday, September 01, 2003

OK, maybe the example was trivial:

How about this part of the some function:

if( varSafe.vt == (VT_ARRAY | VT_VARIANT ) && varSafe.parray != NULL )
{
    if( SafeArrayGetDim(varSafe.parray) == 2 )
    {
        hr = SafeArrayGetUBound( varSafe.parray, 2, &lRows );
        if( SUCCEEDED(hr) && lRows >= 0 )
        {
            SAFEARRAY* psaSafe = varSafe.parray;
            for( long lIndex = 0; lIndex <= lRows; lIndex++ )
            {
                // Do something
            }
        }
    }
}

How big the thing immediately becomes.  What pain of indenting or reading.... Wouldn't this be better

if( varSafe.vt != (VT_ARRAY | VT_VARIANT ) ||
    varSafe.parray == NULL )
    return;

if( SafeArrayGetDim(varSafe.parray) != 2 )
    return;

hr = SafeArrayGetUBound( varSafe.parray, 2, &lRows );
if( !SUCCEEDED(hr) || lRows < 0 )
    return;

SAFEARRAY* psaSafe = varSafe.parray;
for( long lIndex = 0; lIndex <= lRows; lIndex++ )
{
    // Do something
}

OK, apart from style & coding & design perspective etc. is there any other reason??? Some person told me that it was concerned with compiler having to generate some return code etc. etc..... ?? Is this true???

Very very confused
Monday, September 01, 2003

By the way, I use VC++ 6.0

Very very confused
Monday, September 01, 2003

"But if you are creating COM objects you don't have a choice."

Errrm, why? 

Simon Lucy
Monday, September 01, 2003

I prefer the single exit.  Depends on the function though, sometimes that can get too hairy and it looks cleaner to do some checks at the beginning, though when that point is reached, I will usually split it the function up (rather than have like 4 or more levels of indent).

I think the single exit is more of an "academic" style, makes it easier to prove algorithms correct.  It's sort of a more "functional" style -- not quite, I guess, but it feels that way.

Bottom line is that this is not something you should be "very very confused" about. : )  It's pretty much a pure style issue, although there may be something to that point about exceptions (which I haven't used in awhile).  Which way have you been doing it?  Has it caused any problems?  Well, if it ain't broke...

Andy
Monday, September 01, 2003

"But if you are creating COM objects you don't have a choice."

Errrm, why? 

Because I forgot to quote "Personally I dislike the habit of returning constants directly such as return S_OK;" :)

Tony E
Monday, September 01, 2003

if( varSafe.vt != (VT_ARRAY | VT_VARIANT ) || ...

The above code may not be doing what you intend.

Did you mean

if (varSafe.vt != VT_ARRAY && varSafe.vt != VT_VARIANT) ...

(I can't tell the intent from the code, given that I don't have the definitions or semantics of VT_ARRAY and VT_VARIANT, but it looks fishy.  The code may well be correct, however.)

David Jones
Monday, September 01, 2003

The code is right. He wants the variant type to be "SAFEARRAY of VARIANTS". That's indicated by OR-ing the two codes VT_ARRAY and VT_VARIANT together.

Brad Wilson (dotnetguy.techieswithcats.com)
Monday, September 01, 2003

I prefer the multiple return style. If you're worried about readibility, than preface it with a comment:

// Check pre-emptive return conditions.

I think that it's much more readable.  Also, I doubt that there's much difference at the assembly level. I don't know how to generate assembly using VC++, but if you have access to GCC, compile the two versions with the -S flag. I imagine both versions would create similar compares, labels, and jumps.

Nick
Monday, September 01, 2003

I've found that requiring a single exit point makes code harder to write but easier to read. Also, it forces you to write shorter methods, if you want to avoid multiple layers of nested braces in order to keep your code comprehensible.

Julian
Monday, September 01, 2003

I should clarify, I prefer multiple exits points only in the case where you are checking at the beginning of the function for return conditions. If the return points are interspersed throughout the function than that is difficult to read.  If there are a lot of pre-emptive return conditions, I package those in a separate method.

Nick
Monday, September 01, 2003

You should use multiple returns up front when you verify your preconditions. If your preconditions aren't held, your function won't work anyway, and this keeps the verification up front and out of the way of your actual function's logic.

Chris Tavares
Monday, September 01, 2003

I agree with Chris - it's good to have all the parameter sanity checks right up front and exit as soon as possible. Also, don't foget to initialize your OUT params.

For the main body of the function, I prefer this:

HRESULT hr = E_FAIL;

try
{
    // call another function that returns hresult
    hr = foo();
    if ( FAILED(hr) ) AtlThrow( hr );

    // do more work
    ...
 
    hr = S_OK;
}
catch ( CAtlException& e )
{
    hr = e;
}
catch ( ... )
{
}

return hr;

Note: if you mix ATL and MFC and use AtlThrow, you need to catch MFC exceptions in your MFC project (i.e., AtlThrow will throw CMemoryException or CException instead of CAtlException), or just throw CAtlException directly.

pUnk
Monday, September 01, 2003

Use multiple returns, but no more than necessary.  For example, combine all of the checks into one if statement:

if (0
    || varSafe.vt != (VT_ARRAY | VT_VARIANT)
    || varSafe.parray == 0
    || SafeArrayGetDim(varSafe.parray) != 2
    || !SUCCEEDED(SafeArrayGetUBound(varSafe.parray, 2, &lRows)
    || lRows < 0
) {
    return;
}

The 0 beginning the if condition allows all of the real tests to line up nicely.

Alternatively, since you need to do a whole bunch of checking on this input, write a function to do it:

bool
get_variant_array_2d_rows(VARIANT *v, int *rows)
{
    if (1
        && varSafe.vt == (VT_ARRAY | VT_VARIANT)
        && varSafe.parray != 0
        && SafeArrayGetDim(varSafe.parray) == 2
        && SUCCEEDED(SafeArrayGetUBound(varSafe.parray, 2, rows)
        && *rows >= 0
    ) {
        return true;
    }
    else return false;
}

Call it like this:

    int lRows;
    if (get_variant_array_2d_rows(&varSafe, &lRows) {
        for (long lIndex = 0; lIndex < lRows; lIndex++) {
            // Do something
        }
    }

rob mayoff
Tuesday, September 02, 2003

Thanks for all the responses. I sure have learned new things. These responses are very different from what I normally used to get from people I asked the query to.

I've become less confused now :)

Very very confused (A bit better now)
Tuesday, September 02, 2003

Multiple returns are much better in this case as you can tell it's finished when you see the return. Otherwise you have to read all the rest of the code just in case any of it does anything.

JB
Tuesday, September 02, 2003

There's an active thread on this topic on comp.lang.c++.moderated, called "Reasons for single entry, single exit".  It's been going for a few months with over 350 posts, so I would be surprised of the JOS netizens were to arrive at a consensus.  Talk to your coworkers and come to a decision among yourselves.  Both ways are acceptible, and you're unlikely to find an objective basis for choosing one style over the other [with the possible exception of RVO opportunities].  You might as well debate brace indentation.

Brian
Tuesday, September 02, 2003

In many cases I prefer multiple return points because that way you can easily tell when reading that the function is bailing out and not continuing to do work.

Someone mentioned that single return point is more "functional" style, I would disagree.  Since pure functional languages make much less use of assignment, functional programs typically have multiple return points rather than assigning the different return values to a local variable and then returning the variable

Mike McNertney
Tuesday, September 02, 2003

*  Recent Topics

*  Fog Creek Home