Multiple return values to indicate success/failure.

Question

I'm kind of interested in getting some feedback about this technique I picked up from somewhere.

I use this when a function can either succeed or fail, but you'd like to get more information about why it failed. A standard way to do this same thing would be with exception handling, but I often find it a bit over the top for this sort of thing, plus PHP4 does not offer this.

Basically the technique involves returning true for success, and something which equates to false for failure. Here's an example to show what I mean:

define ('DUPLICATE_USERNAME', false);
define ('DATABASE_ERROR', 0);
define ('INSUFFICIENT_DETAILS', 0.0);
define ('OK', true);

function createUser($username) {
    // create the user and return the appropriate constant from the above
}

The beauty of this is that in your calling code, if you don't care WHY the user creation failed, you can write simple and readable code:

if (createUser('fred')) {
    // yay, it worked!
} else {
    // aww, it didn't work.
}

If you particularly want to check why it didn't work (for logging, display to the user, or do whatever), use identity comparison with ===

$status = createUser('fred');
if ($status) {
    // yay, it worked!
} else if ($status === DUPLICATE_USERNAME) {
    // tell the user about it and get them to try again.
} else {
    // aww, it didn't work. log it and show a generic error message? whatever.
}

The way I see it, the benefits of this are that it is a normal expectation that a successful execution of a function like that would return true, and failure return false.

The downside is that you can only have 7 "error" return values: false, 0, 0.0, "0", null, "", and (object) null. If you forget to use identity checking you could get your program flow all wrong. Someone else has told me that using constants like an enum where they all equate to false is "ick".

---

So, to restate the question: how acceptable is a practise like this? Would you recommend a different way to achieve the same thing?

Answer 1

As long as it's documented and contracted, and not too WTFy, then there shouldn't be a problem.

Then again, I would recommend using exceptions for something like this. It makes more sense. If you can use PHP5, then that would be the way to go. Otherwise you don't have much choice.

Answer 2

A more common approach I have seen when exceptions aren't available is to store the error type in a 'last_error' variable somewhere and then when a failure happens (ie it returns false) look up the error.

Another approach is to use the venerable unix tool approach numbered error codes - return 0 for success and any integer (that maps to some error) for the various error conditions.

Most of these suffer in comparison to exceptions when I've seen them used however.

Just to respond to Andrew's comment - I agree that the last_error should not be a global and perhaps the 'somewhere' in my answer was a little vague - other people have suggested better places already so I won't bother to repeat them

Answer 3

Often you will return 0 to indicate success, and 1,2,3 etc to indicate different failures.
Your way of doing it is kinda hackish, because you can only have so many errors, and this kind of coding will bite you sooner or later.

I like defining a struct/object that includes a Boolean to indicate success, and an error message or other value indicate what kind of error occurred. You can also include other fields to indicate what kind of action was executed.
This makes logging very easy, since you can then just pass the status-struct into the logger and it will then insert the appropriate log entry.

Answer 4

@mmaibaum

return 0 for success and any integer (that maps to some error) for the various error conditions.

yeah, i had considered that, but it does mean that if you might see this in your code:

if (!createUser('fred')) {
    // it worked...
}

That seems somewhat counter-intuitive to me, I dunno if it's just that I'm not used to it.

Answer 5

Ick.

In Unix pre-exception this is done with errno. You return 0 for success or -1 for failure, then you have a value you can retrieve with an integer error code to get the actual error. This works in all cases, because you don't have a (realistic) limit to the number of error codes. INT_MAX is certainly more than 7, and you don't have to worry about the type (errno).

I vote against the solution proposed in the question.

Answer 6

Look at COM HRESULT for a correct way to do it.

But exceptions are generally better.

Update: the correct way is: define as many error values as you want, not only "false" ones. Use function succeeded() to check if function succeeded.

if (succeeded(result = MyFunction()))
  ...
else
  ...

Answer 7

I agree with the others who have stated that this is a little on the WTFy side. If it's clearly documented functionality, then it's less of an issue, but I think it'd be safer to take an alternate route of returning 0 for success and integers for error codes. If you don't like that idea or the idea of a global last error variable, consider redefining your function as:

function createUser($username, &$error)

Then you can use:

if (createUser('fred', $error)) {
    echo 'success';
}
else {
    echo $error;
}

Inside createUser, just populate $error with any error you encounter and it'll be accessible outside of the function scope due to the reference.

Answer 8

If you really want to do this kind of thing, you should have different values for each error, and check for success. Something like

define ('OK', 0);

define ('DUPLICATE_USERNAME', 1);

define ('DATABASE_ERROR', 2);

define ('INSUFFICIENT_DETAILS', 3);

and check

if (createUser('fred') == OK) {

//ok

} else {

//fail

}

Answer 9

It does make sense that a successful execution returns true. Handling generic errors will be much easier:

if (!createUser($username)) {
// the dingo ate my user.
// deal with it.
}

But it doesn't make sense at all to associate meaning with different types of false. False should mean one thing and one thing only, regardless of the type or how the programming language treats it. If you're going to define error status constants anyway, better stick with switch/case

define(DUPLICATE_USERNAME, 4)
define(USERNAME_NOT_ALPHANUM, 8)

switch ($status) {
case DUPLICATE_USERNAME:
  // sorry hun, there's someone else
  break;
case USERNAME_NOT_ALPHANUM:
  break;
default:
  // yay, it worked
}

Also with this technique, you'll be able to bitwise AND and OR status messages, so you can return status messages that carry more than one meaning like DUPLICATE_USERNAME & USERNAME_NOT_ALPHANUM and treat it appropriately. This isn't always a good idea, it depends on how you use it.

Answer 10

how acceptable is a practice like this?

I'd say it's unacceptable.

1. Requires the === operator, which is very dangerous. If the user used ==, it leads to a very hard to find bug.
2. Using "0" and "" to denote false may change in future PHP versions. Plus in a lot of other languages "0" and "" does not evaluate to false which leads to great confusion

Using getLastError() type of global function is probably the best practice in PHP because it ties in well with the language, since PHP is still mostly a procedural langauge. I think another problem with the approach you just gave is that very few other systems work like that. The programmer has to learn this way of error checking which is the source of errors. It's best to make things work like how most people expect.

if ( makeClient() )
{ // happy scenario goes here }

else
{
    // error handling all goes inside this block
    switch ( getMakeClientError() )
    { case: // .. }
}

Answer 11

When exceptions aren't available, I'd use the PEAR model and provide isError() functionality in all your classes.

Answer 12

I like the way COM can handle both exception and non-exception capable callers. The example below show how a HRESULT is tested and an exception is thrown in case of failure. (usually autogenerated in tli files)

inline _bstr_t IMyClass::GetName ( ) {
    BSTR _result;
    HRESULT _hr = get_name(&_result);
    if (FAILED(_hr)) _com_issue_errorex(_hr, this, __uuidof(this));
    return _bstr_t(_result, false);
}

Using return values will affect readability by having error handling scattered and worst case, the return values are never checked by the code. That's why I prefer exception when a contract is breached.

Answer 13

Other ways include exceptions (throw new Validation_Exception_SQLDuplicate("There's someone else, hun");), returning structures (return new Result($status, $stuff); if ($result->status == 0) { $stuff = $result->data; } else { die('Oh hell'); }.

I would hate to be the person who came after you for using the code pattern you suggested originally.

And I mean "Came after you" as in "followed you in employment and had to maintain the code" rather than "came after you" "with a wedgiematic", though both are options.

Answer 14

Reinventing the wheel here. Using squares.

OK, you don't have exceptions in PHP 4. Welcome in the year 1982, take a look at C.

You can have error codes. Consider negative values, they seem more intuitive, so you would just have to check if (createUser() > 0).

You can have an error log if you want, with error messages (or just arbitrary error codes) pushed onto an array, dealt with elegance afterwards.

But PHP is a loosely typed language for a reason, and throwing error codes that have different types but evaluate to the same "false" is something that shouldn't be done.

What happens when you run out of built-in types?

What happens when you get a new coder and have to explain how this thing works? Say, in 6 months, you won't remember.

Is PHP === operator fast enough to get through it? Is it faster than error codes? or any other method?

Just drop it.