Is it bad practice to have multiple return statements in a function or method?

[unregistered]

will having multiple returns make it more unreadable in contrast to having just one return statement as the last line in a function/method?

[jsmith]

Yes and no. This turned into a religious debate on another forum elsewhere. Some say: "With multiple returns, it's hard to tell what executes and what doesn't, and can lead to bugs with cleanup code not running due to an early return that should have run." I say: "Functions should be small, first off. Secondly, it's still hard to tell what code executes and what doesn't because you have to cause execution to 'snake' its way through to the bottom of the function. Thirdly, proper use of RAII and resource manager objects eliminates the cleanup code problem automatically, and such techniques are required anyway, if the code is to be exception safe."

Examples:

struct Person {
    std::string firstName;
    std::string lastName;
};

// I prefer this, with an "early return"
bool IsNameFound( const std::deque<Person>& people, const std::string& first, const std::string& last ) {
    typedef std::deque<Person>::const_iterator Iter;

    for( Iter i = people.begin(); i != people.end(); ++i )
        if( boost::bind( &Person::firstName, _1 ) == first && boost::bind( &Person::lastName, _1 ) == last )
            return true;

    return false;
}

// Others prefer this
bool IsNameFound( const std::deque<Person>& people, const std::string& first, const std::string& last ) {
    typedef std::deque<Person>::const_iterator Iter;

    bool found = false;
    for( Iter i = people.begin(); i != people.end() && !found; ++i )
        if( boost::bind( &Person::firstName, _1 ) == first && boost::bind( &Person::lastName, _1 ) == last )
            found = true;

    return found;
}

Personally, I don't find the second as understandable as the first because the second requires me to understand how the "found" variable works. Now in this case, it really isn't any harder -- it's kinda obvious -- but in more complex examples it can be harder. Consider a slightly more complex example:

typedef std::vector< std::vector< std::string > > String2DVec;

// My preferred way, with an 'early return':
bool ReplaceString( String2DVec& vec, const std::string& oldStr, const std::string& newStr ) {
    typedef String2DVec::value_type::iterator VIter;

    for( String2DVec::iterator i = vec.begin(); i != vec.end(); ++vec )
        for( VIter j = i->begin(); j != i->end(); ++j )
            if( *j == oldStr )
            {
                 *j = newStr;
                 return true;
             }

    return false;
}

// Others prefer:
bool ReplaceString( String2DVec& vec, const std::string& oldStr, const std::string& newStr ) {
    typedef String2DVec::value_type::iterator VIter;

    bool found = false;
    for( String2DVec::iterator i = vec.begin(); i != vec.end() && !found; ++vec )
        for( VIter j = i->begin(); j != i->end(); ++j )
            if( *j == oldStr )
            {
                 *j = newStr;
                 found = true;
                 break;
             }

    return found;
}

The statement "return found;" is very well self-documenting, but the logic to get there in my opinion is a little roundabout. And still others would write it even more convoluted (imo):

bool ReplaceString( String2DVec& vec, const std::string& oldStr, const std::string& newStr ) {
    typedef String2DVec::value_type::iterator VIter;

    bool found = false;
    for( String2DVec::iterator i = vec.begin(); i != vec.end(); ++vec )
    {
        for( VIter j = i->begin(); j != i->end(); ++j )
            if( *j == oldStr )
            {
                 *j = newStr;
                 found = true;
                 break;
             }

        if( found )
            break;
    }

    return found;
}

EDIT: and then there are still other purists who say "break is nothing more than a goto" (no goto wars here, please):

bool ReplaceString( String2DVec& vec, const std::string& oldStr, const std::string& newStr ) {
    typedef String2DVec::value_type::iterator VIter;

    bool found = false;
    for( String2DVec::iterator i = vec.begin(); i != vec.end() && !found; ++vec )
    {
        for( VIter j = i->begin(); j != i->end() && !found; ++j )
            if( *j == oldStr )
            {
                 *j = newStr;
                 found = true;
             }
    }

    return found;
}

I prefer concise implementations to verbose ones.

[jsmith]

There is also RVO to consider (in C++). Return Value Optimization allows the compiler to return a value directly to where the callee stores it rather than into a temporary which then gets copied. So for example:

std::string frobnicate( const std::string& s1, const std::string& s2 )
{
    std::string result;

    if( s1.empty() && s2.empty() )
        result = "both strings are empty!";
    else if( s1.empty() )
        result = "s1 is empty, but s2 is '" + s2 + "'";
    else if( s2.empty() )
        result = "s2 is empty, but s1 is '" + s1 + "'";
    else
        result = s1 + " " + s2;

    return result;
}

std::string answer = frobnicate( "Hello", "World" );

In the above code, the compiler is likely to allocate a std::string on the stack to hold result, manipulate result, return it, then copy the returned value to "answer".

But if we do this, causing those "no-early-returners" to cringe:

std::string frobnicate( const std::string& s1, const std::string& s2 )
{
    if( s1.empty() && s2.empty() )
        return "both strings are empty!";
    else if( s1.empty() )
        return "s1 is empty, but s2 is '" + s2 + "'";
    else if( s2.empty() )
        return "s2 is empty, but s1 is '" + s1 + "'";
    else
        return s1 + " " + s2;
}

std::string answer = frobnicate( "Hello", "World" );

The compiler is likely to construct the std::string that is returned directly in answer, avoiding the temporary and the copy. This increases peformance, particularly when you start returning non-trivial things such as containers (to those who say 'always return containers through reference parameters' I say 'the calling convention of such a function sucks since I must now write two lines of code instead of one; let rvo do it's thing.')

[moorecm]

Two comments:
1) First example, line 26, you mean return found;
2) Watch that string literal + std::string stuff!

[jsmith]

Fixed #1 - thanks. #2 should be fine -- literals will be implicitly converted to std::string since operator+ taking two std::strings is supported.

[moorecm]

Oops! For some reason I was thinking of string literal + integer...

[PanGalactic]

I found this quote a long time ago:

In languages with exceptions, any function that can either throw or return "normally" has multiple exit points, and once you accept that, there's actually almost zero additional complexity in returning when it's appropriate; it can twist code to make it into SESE style.

-- http://thedailywtf.com/Comments/Nothing_Final.aspx?pg=3#119208

SESE make no sense in C++. But SESE still makes sense for C, which lacks destructors and RAII. One must clean up resources manually. A common C idiom (and one of the few accepted uses of GOTO) is the "goto end;" statement, which is used to release resources allocated within the function before the single return.

http://www.cprogramming.com/tutorial/goto.html