Return diamonds, not coal!

The following code makes me shutter:

boolean Foo ()
{
    // do stuff
    return true;
}

int Foo2 ()
{
    // do stuff
    return 1;
}

I have long been a proponent of returning status from a method, but the code above is useless.

The methods above have no business returning ‘true’ or ‘1’, because these responses are meaningless.  You may be thinking that the answer is obvious: the return values are static and are not subject to change.  If there was an opportunity for variability, like returning ‘true’ or ‘false’, then the returned boolean would have meaning.  I would argue that this is only a partial solution.

Let me explain.

Returning ‘true’ or ‘false’ is a useless activity.  Keeping with the theme, ‘-1’, ‘0’, & ‘1’ are also useless.  Obviously this does not apply to methods whose responsibility requires that they return a numerical value (e.g. int GetCurrentCount()) or a boolean result (e.g. boolean IsThisEnabled()).  This argument applies to those methods that return numerical or boolean values in place of status.

Let me ask you a simple question:  what does ‘-1’ mean?  Your response may be something like ‘nothing’, ‘not found’, ‘ not initialized’, or ‘error’.  That is my point, it has many meanings.  You have to look at the context under which it is used to understand its meaning.  This on-the-fly translation leads to complexity because the reviewer must remember its meaning under the context of the code s/he is reviewing.  Since ‘-1’ depends on the context, and the meaning of ‘-1’ could be different between coders, the meaning of ‘-1’ must be constantly re-interpreted by the person reviewing the code.

Now this may seem like a frivolous argument to you.  You may be thinking, ‘so what’.  We will just standardize ‘-1’ to mean ‘error’.  You may decide to define a macro to encode the meaning, and use the macro instead of the value (e.g. #define ERROR -1;).  But this mindset is akin to ‘putting lipstick on a pig’; no matter how much you dress it up, you still have the same problem: ‘true’, ‘false’, ‘-1’, ‘0’, & ‘1’ are meaningless when used to reflect status.

Time for a thought experiment.

Let us say you have the following code:

 

if (ERROR == pClass->Method())
{
    // handle the error
}

public int Class::Method()
{
    // do stuff

    return ERROR; // where ERROR is defined as -1
}

This code works.  There is no functional problem with it.  To see the problem you have to look beyond the function of the code & interpret the philosophy behind it.  The philosophy is "Class::Method encountered an error, so you handle it".  This is not a good design.

To understand why, think about how you would handle the error.  Without any additional information about the error, you are left with a generic handler for the problem.  Sure, you could log it & inform the user.  Perhaps you decide to stop whatever processing you were doing, clean it up, & exit.  However, ask yourself, did you truly handle the error?  In reality, you did not.  You only mitigated it.  Mitigating the error creates a situation where depended code is tightly coupled to all potential responses provided from the method.

Without context, you cannot properly handle the response.  There is not enough meaning behind the responses to have dependent code properly handle what is receives.  This issue becomes evident when the method could potentially respond with a second return value.  Are you willing to go back to every call to ‘Method’ and double check if the new return value needs to be handled?  If a new response is added, and you do not update the dependent code to handle it, you are ignoring potential responses which dependent code should handle.  The dependent code remains ignorant and assumes everything is fine.

Return values should not force the caller to interpret the response. 

Here is how the code should read:

 

pClass->Method();


public void Class::Method()
{
    // do stuff

    if (an_error_occurred)
    {
        // handle the error
    }

    // -- or --

    AnErrorOccurred:
        // handle the error
}

‘Handle the error’ means whatever your environment will allow.  It could be as simple as logging the error and throwing an exception.  Adding code to handle the error at the point of failure, where there is context for the error, results in the following:

Result	Benefit
Clear indication of where the failure occurred	Easier debugging since you now know where to look
Error handling is contained under the context which the error occurred	Clarity of the problem due to more available information Dependent code does not need to process & potentially misinterpret the response from the method Removes the potential for the response to be ignored by dependent code
Code which depends on the response does not need to be updated when the context changes	Less likely that the program will continue operating under the assumption that everything is ok Do not have to update dependent code whenever the response changes or a new response is added

Though my example focuses on returning an error, the philosophy applies to anything that could be used as a response, especially with ambiguous information like a boolean or number. 

Using this approach you refine the information internally before concluding that a response is needed, which leads to meaningful, valuable responses (i.e. diamonds).  Dependent code will not need to interpret a vague response without context (i.e. coal) to make a decision on what to do next.  The philosophy of the design has changed from "I got a response, what does it mean" to "I got a response, what action do I need to take".  The difference is subtle but powerful.

————–

If you find yourself in a method that has a response, ask yourself the following:

Q1: Does it need to respond with anything at all?

Q2: Can you handle it internally?

Q3: Will the next engineer or software system, which relies on the code, always understand the definition of the response?

Q4: Will dependent code need to be updated to handle a new or modified response? 

Q5: Can you handle it internally? (had to ask this twice)