Return Value for "should cancel"

Question

I have a method DoCleanUp(), which will ask user to proceed and then clear current workspace. It will return if user choose to cancel this process.

My question is, which signature is best to indicate a "cancel"?

1. bool DoCleanUp(); // return false to indicate canceled.

2. bool DoCleanUp(); // return true to indicate this method should be canceled.

3. void DoCleanUp(bool& cancel); // check parameter 'cancel' to see if this method was canceled.

UPDATE: As for the language, it's C++\CLI or C#.

UPDATE2: Now suppose I have to save a file in the DoCleanUp method. I'll prompt a dialog ask user whether to save/not save/cancel the file. Based on the answers, here is what I came up:

void DoCleanUp();

DialogResult AskToSaveFile(); // return yes/no/cancel

void DoCleanUp( bool saveFile );

Usage:

void DoCleanUp()
{
    DialogResult result =  AskToSaveFile();

    if( result == DialogResult::Cancel )    return; 

    bool saveFile = (result == DialogResult::Yes) ? true : false;
    DoCleanUp( saveFile );
}

Then by calling DoCleanUp(), you know user will have the opportunity to cancel;
By calling DoCleanUp(bool saveFile), you can control whether to save file without asking user.
Is that looks better?

Answer 1

I believe option three gives the most clarity. When you have the bool as a return type it is not immediately clear what it is used for.

Answer 2

I usually go with

 bool DoCleanUp();  // Returns true if cancel

but mostly it depends on whether the calling code looks like this:

 if (DoCleanUp()) {
     // Do cancel up code
 }

or:

 if (DoCleanUp()) {
     // Do non-cancel post clean up code
 }

Basically I try to make my tests not have to use a ! or language equivilent as I find it hard to see.

I definitely would not do number 3.

Answer 3

I prefer the third signature, only because by looking at it (without any extra documentation), I can tell more about what the method does. I would call the argument something more explicit, like processCancelled, though.

Answer 4

I would use your 1 version.

bool DoCleanUp(); // return false to indicate canceled.

The assumption is, that it returns true when the cleanup is done. Returning false would indicate a 'Error' state. It might even make sense to return an int. In this case the convention usually is that 0 represents success and everything else is an error code.

Regardless of what you decide, document what your return values mean!

Answer 5

Without more details I would say answer 1 is the best IMHO. Third is rather ugly since it requires more code for calling.

But maybe consider rewriting code to this

void CleanUp() {
   switch (AskUser()) {
     case ButtonOk: CleanUpDesk(); break;
     case ButtonNo: break;
     default:
     case ButtonCancel: CancelCleanUpDesk(); break;
   }
}

This seems to in the spirit of single responsibility. My code somehow breaks your problem into two steps: asking user and performing action.

Answer 6

This is a classic single responsibility problem.

The reason that you are unsure about the signature is that the method is doing 2 things.

I would create 2 methods:

bool CheckIfTheUserWantsToCancel()
void DoCleanUp()

EDIT

Based on the comments and edits to the question I would create a 3rd method:

void SaveFile()

The DoCleanUp would then first call CheckIfTheUserWantsToCancel, and then if not cancelled would call SaveFile.

IMHO this is much better than trying to remember that DoCleanUp with parameter false will save the file without asking the user, or was it the other way around?

Answer 7

The confusing bit is the calling it DoSomething(), when it might not do anything. How about

if (QueryCleanup())     // boolean
    DoCleanup();        // void

More verbose but clearer, even without seeing the declaration.

Answer 8

You should not use a boolean for statuses (or status messages). Create an Enum:

public Enum CleanupStatus
{
    Ok = 0,
    Cancel
}

This way it is more clear what the return value is ... and if you need to add more statuses, you can.

(This is all from Code Complete 2, you should read it if you haven't yet.)

Answer 9

You have two requests basically. The outer request is to create a new workspace. The inner request is to save the current workspace. You want to return true if the outer request continues and false if the outer request is aborted. The action of the inner request is not important to the outer request and so should be some kind of delegate/functor/closure.

Make a class to genericize this:

class YesNoCancel {
   string question; // question to ask the user about the inner state
   delegate doit; // function to call to 
   delegate dontdoit;
public:
   YesNoCancel(string question, delegate doit, delegate dontdoit = null) {...}

   bool run() {
     switch (AskUser(question)) {
     case ANSWER_YES: doit(); return true;
     case ANSWER_NO: return true;
     case ANSWER_CANCEL: if (dontdoit) dontdoit(); return false;
};

//usage

void NewWorkspace() {
    if (m_workspace) {
        YesNoCancel ync("Save current workspace?", saveworkspace);
        if (!ync.run()) return;
    }
    // new workspace code
}

void CloseApp() {
    YesNoCancel ync("Save current workspace?", saveworkspace);
    if (ync.run()) ExitApplication();
}