return statements when doing Extract Method

Question

Let's say you have a very long method, like this:

int monster()
{
    int rc = 0;

    // some statements ...

    if (dragonSlayer.on_vacation()) {
        cout << "We are screwed!\n";
        if (callTheKing() == true)
            return 1;
        else
            return 2;
    } else {
        cout << "We are saved!\n";
        slayTheDragon();
    }

    // rest of long method...

    return rc;
}

and I'm working on skeletonizing the code. I want to extract the dragon slaying part to

int handleDragon() {
    if (dragonSlayer.on_vacation()) {
        cout << "We are screwed!\n";
        if (callTheKing() == true)
            return 1;
        else
            return 2;
    } else {
        cout << "We are saved!\n";
        slayTheDragon();
    }

    return 0; // ?
}

and replace the code in monster() with a call to handleDragon().

But there is a problem. There is a return statement in the middle of that part. If I keep the part where the return code of handleDragon() is handled, it will keep the litter in the big method.

Besides using exceptions, is there an elegant and safe way to refactor this piece of code out of the monster method? How should these types of situations be handled?

Answer 1

Return 0 from the handleDragon method if the dragon slayer is available:

int handleDragon() {
    if (dragonSlayer.on_vacation()) {
        cout << "We are screwed!\n";
        if (callTheKing() == true)
            return 1;
        else
            return 2;
    } else {
        cout << "We are saved!\n";
        slayTheDragon();
        return 0;
    }
}

Then back in the monster method, if the return value was greater than zero, return that value, otherwise carry on:

// some statements ...

int handleDragonResult = handleDragon();
if (handleDragonResult > 0) {
    return handleDragonResult;
}

// rest of long method...

You should also document the handleDragon method, to explain the value that gets returned.

Answer 2

Couldn't you do this:

int handleDragon() {
    int rc = 0;

    if (dragonSlayer.on_vacation()) {
        cout << "We are screwed!\n";
        if (callTheKing() == true)
            rc = 1;
        else
            rc = 2;
    } else {
        cout << "We are saved!\n";
        slayTheDragon();
    }

    return rc;
}

and then:

int monster()
{
    int rc = 0;

    // some statements ...

    rc = handleDragon();

    // rest of long method... 

    return rc;
}

or if you want to do something with the return code:

int monster()
{
    int rc = 0;

    // some statements ...

    int handleDragonReturnCode = handleDragon();

    if(handleDragonReturnCode == 0) {
       // do something
    }

    else {
      // do something else
    }

    // rest of long method... 

    return rc;
}

Is this what you want? On a general note, avoid using magic numbers like 1 and 2 for your return codes. Use constants, #define, or enum.

Concerning return, try to have one exit point from your function. As you have found out, having multiple return statements can make refactoring hard (as well as understanding the logic unless it's really simply).

Answer 3

enum DragonHandled { DHSuccess, DHKing, DHNoKing };

inline DragonHandled askForKing()
{
    if (callTheKing())
        return DHKing;
    else
        return DHNoKing;
}

DragonHandled handleDragon()
{
    if (dragonSlayer.on_vacation()) {
        cout << "We are screwed!\n";
        return askForKing();
    }
    cout << "We are saved!\n";
    slayTheDragon();
    return DHSuccess;
}

int monster()
{
    some_statements(...);

    DragonHandled handled = handleDragon();
    if( handled != DHSuccess )
      return handled; // enum to int is an implicit cast

    return more_statements(...);
}

• Except for a function that returns an actual signed number, I would not return int. If the result has a meaning, define that meaning properly (that is: an enum).
• A function does something, and whatever it does, should be visible in its name. So there should be a verb in a function's name (handledragon(), callTheKing()). monsters isn't a verb, it isn't something you can do. If I see an identifier monsters, I'd think it's a container for monsters.
• Checking if(x == true) is just useless noise, since if(x) is terser, simpler and just as true.

Answer 4

The question was about the strategy so I think the answer by Richard Fearn is a good one.

To make it into a refactoring pattern it would look something like:

Context: A section in the middle of a larger method is to be extracted.

Problem: The section contains return statements.

Solution:

1. Extract the code to a new method returning the same type as the larger method.
2. Find a value of that type that does not mean anything. Call that value CONTINUE.
3. Add a statement at the end of the new method that returns CONTINUE.
4. In the larger method test the return value from the new method for CONTINUE. If it is not then return that value.

This would be the principal approach. As the next step you could refactor the return values from the new method to something more meaningful (like in the answer from sbi). And you'd have to find a way to handle the case where the return type isn't a scalar or simple type, returning a NULL object or some such.