[creativecoding]

Hello all. Lately I've been taking advantage of the ability to return different types of data. Mainly what I'd do is return true on success, false on unknown failure (something like a database connection problem, where I don't want the exact error out in public), or a string for a special message. Here's an example function:

function register($user, $pass){
    if(strlen($user) < 4 || strlen($pass) < 4){
        return "Your username must be over 3 characters.";
    }
    $result = some database registration stuff, true on row inserted false on none inserted. 
    return $result;
}

And then on the register process page:

<?php
$result = register($_POST['user'], $_POST['pass']);
if($result === true){
    echo "You have been registered."
} else if($result === false){
    echo "An unknown error has occurred. Please try again later.";
} else {
    echo $result;
}
?>

I was wondering if this is good coding practice or not. I also make sure to document the function and what it returns.

[Martyr2]

Yeah this one is a toughy and I try to avoid such situations by refactoring this. What you are really asking is two different things right? One is that you are checking the validity of the username and the second is if the registration was successful. I avoid this situation by making a function that returns true/false if the name is valid or not and the the second function to do the registration... which again could be true/false. I would then print the error message near the caller.

if (isValidUsername($username)) {
    if (register($username,$pass)) {
         // Success message
    }
    else {
         // Failure message
    }
}

But of course this is preference and honestly I can't say it is exactly a bad practice because PHP itself uses this style with many of its internal functions. Many functions will return a string on success and false on error.

One thing to keep in mind however is how versatile is your function if it is printing messages. Sure your failure message may work in one design situation, but what if you want to use this function in other areas of the application but with a different error message? You lose a little flexibility here. Register() should be concerned with the registration process code, your caller can worry about what to print based on the result returned from register().

Just a few tips but I can't say it is bad practice. More a design consideration. As long as you document why you did it this way and make it easy for someone to change it later (like rip out your function for a refactor) there should be nothing wrong.

[Dormilich]

it is better practice than die()ing in any case.

though from personal experience I can recommend the use of exceptions. they allow for different types of errors (RuntimeException, UnexpectedValueException, PDOException, to name only some) and provide the possibility to hide the error message as well. and noteably, you can set PDO to use exceptions so that you don’t have to do it by hand.

esp. with objects I use return values for the methods most of the times as well (independently of throwing exceptions)

[AdaHacker]

creativecoding, on 15 January 2012 - 04:00 PM, said:

Mainly what I'd do is return true on success, false on unknown failure (something like a database connection problem, where I don't want the exact error out in public), or a string for a special message.

I would recommend against doing things like that. For starters, as Dormilich pointed out, exceptions offer a much better mechanism for returning information about an error. What you have here is the poor-man's version of exceptions.

Secondly, Martyr2 is correct - your validation messages don't belong in the same place as your back-end database stuff. You need to separate your concerns.

And lastly, while mixing return types is not inherently bad, you're coding against the common idiom of the language, which is almost always a bad idea. When a function returns both booleans and non-booleans, we typically expect all "truthy" values to be equivalent in some sense, i.e. the odd-ball data type should be the exception (think of things like strpos()). Having both non-empty strings and boolean false indicate error conditions feels weird and is likely to be confusing down the line.

Martyr2, on 15 January 2012 - 04:15 PM, said:

But of course this is preference and honestly I can't say it is exactly a bad practice because PHP itself uses this style with many of its internal functions.

Actually...that's a pretty good indication that this is a bad practice. The PHP standard library is an unholy mess. It's hardly something to be emulating.

Also, you have to keep in mind that PHP didn't get exceptions until version 5. Most of the functions in the standard library were added before that, so the only methods of error reporting available were the trigger_error() (which is absolutely terrible for routine errors) or returning a special value. In addition, much of the standard library is made up of very thin wrappers around C libraries, and C doesn't have exceptions either. So really, the standard library was written for a different world. Modern PHP has much better error handling facilities and there's no reason not to use them.

[e_i_pi]

The previous posters are much more experienced than I am, but I'll throw in my two cents.

I use Exceptions as well to halt bad execution / cater for unknown errors. I also throw exceptions on errors (created commonality in error reporting), using the following piece of code:
(Forgive the fatal error handler, I don't believe the ErrorLog::Submit method can possibly work at the point of shutdown, but I haven't reviewed this code for a while)

function ErrorHandler($errno, $errstr, $errfile, $errline)
{
	if(0 == error_reporting()) return true;
	throw new Exception('File: ' . $errfile . '; Line: ' . $errline . '; Error: ' . $errstr, $errno);
}


function ExceptionHandler($e)
{
	return ErrorLog::Add($e, 'ExceptionHandler');
}


// Handles Fatal Errors, such as function names being misspelt
function FatalErrorHandler()
{
	ErrorLog::Submit();
	if (is_null($err = error_get_last()) === FALSE)
	{
		$error = 'FATAL ERROR HAS OCCURRED' . PHP_EOL;
		$error .= "Type: " . $err['type'] . PHP_EOL;
		$error .= "Message: " . $err['message'] . PHP_EOL;
		$error .= "File: " . $err['file'] . PHP_EOL;
		$error .= "Line: " . $err['line'] . PHP_EOL;
		ob_start();
		debug_print_backtrace();
		$output_buffer = PHP_EOL . ob_get_contents();
		ob_end_clean();
		error_log($output_buffer);
		error_log($error);
	}
}

...which is then registered using this in my config file ( a common file included in all script executions)...

set_error_handler('ErrorHandler');
set_exception_handler('ExceptionHandler');
register_shutdown_function('FatalErrorHandler');

The reference to the ErrorLog class is one of Dormilich's gems, located here.

-------------------------------------------------

Where returned value types gets interesting (read: painful) is when you are communicating with the client via AJAX. Your AJAX function will expect the response content-type to be HTML*
^{* Different response types are available, such as XML, but for argument's sake, let's assume that all server responses are coming back as HTML}

If your server is running into some sort of error, and instead of returning HTML (i.e. - a string) it is returning false (i.e. - a boolean) then AJAX will blindly just accept that as the response, and attempt to display the repsonse in the manner you have instructed.

Getting around this can be painful, as you may well want your code to return boolean, but your AJAX to receive string. The way I get around this is by having AJAX call a front controller, which handles any script response before returning the script response to the client.

The basic premise is this:

• The server receives an AJAX request, targetted at it's front controller
  
• The front controller determines (from $_POST variables) which class/method/function/etc is being requested
  
• Let us suppose the called script fails at this point, whether by DbC, bad DB connection, exceptions, errors, etc!
  
• The failed script calls a singleton class that is responsible for logging all errors that require a user-response
  
• The failing script, by way of your code methodology, returns false to the front controller
  
• The front controller checks to see if the singleton error-log is empty or not
  
• If it is empty, then return the script response
  
• If it is not empty, generate a popup that contains the error information that the user needs to know, and return this instead of the script response.

I in fact do a little more than this (to cater for certain framework-specific scenarios), but that is the basic pattern I use for returning information to the user, when I don't know how/where this information (read: response) will be displayed within the HTML DOM.

A good example would be someone posting a reply to a comment on Facebook, but in the course of them sending the request, the comment is removed (i.e. - there is no longer a target DB row for the reply to attach to). The graceful way for this to fail would be to return a popup message to the user, rather than errors over the screen, or slient response (read: AJAX trying to display a PHP false response).

[creativecoding]

Thanks for all of the replies, each and every one are VERY helpful. I wish I could upvote more than once. I'll definitely be looking into Exceptions from now on as a form of returning data.

[Dormilich]

when it comes to AJAX I use headers to indicate errors (maybe except fatal errors). easily available through xhr.status.

[e_i_pi]

I didn't know you were able to set XHR status from server side. That would have saved me a lot of pain a month back lol.

If I have responses that are coming back from the server that I want the AJAX function to handle differently to it's usual success method, I prefix the response with "###ERR", and then conditionally determine what to do with the response based on whether the first 6 characters are that string.

[Dormilich]

jQuery offers the statusCode setting to deal with different status codes.

[e_i_pi]

+1 Dormilich. I never thought of doing it that way - it's a much better solution

For the sake of anyone who is interested in returning server status codes, an example in PHP is:

header('HTTP/1.1 200 OK');

[PHP Manual entry on the header() function]

And here's a list of server status codes according to Wikipedia, W3, and RFC.

[snetty]

I'd echo the other guys that have mentioned Exceptions. Don't forget that you can extend the Exception class to provide for different situations.

For example, it's very handy to use in validation classes.

This post has been edited by Dormilich: 05 June 2012 - 06:55 PM

[cpumatt]

I think it's defiantly better than making all of those 'if' statements. I think I'll use your practice now. Thanks!

I wrote that totally wrong. I meant to say:

My version of doing this would to recklessly put 'if' statements on the page and then at the end putting "You've been registered".

I think putting it into a function makes the code seem much more organized.