How would you check that script parameters are correct and return an error msg?

[Anonim250]

Hello,

 

Here is my initial code:

 

function GetPageContent()
{
if(!isset($_GET['userid']) || !isset($_GET['key']))
	return 'Wrong parameters!';

$userID = $_GET['userid'];
$user = UserAccessor::GetById($userID);
if($user == null)
	return 'Wrong user id!';

if($_GET['key'] != GenerateKeyFromUser($user))
	return 'Wrong key!';

// main action starts here

 

I don't like that the function is bloated with such checks. It becomes harder to read and I think that these checks should be extracted to a separate function. So I think it should be something like this:

 

function GetPageContent()
{
if(!ParametersAreValid())
	return $error;

// main action starts here

 

2 options pop up on my mind:

 

1.

function GetPageContent()
{
list($parametersAreCorrect, $errorMsg) = CheckParameters();
if(!$parametersAreCorrect)
	return $errorMsg;

// main action starts here

 

2.

function GetPageContent()
{
try
{
	CheckParameters();
}
catch(Exception $ex)
{
	return $ex->getMessage();
}

// main action starts here

 

But I don't like option 2 because I think that in this situation exception is used to control program flow. And even if we say that wrong parameters is an exceptional situation here, this code will not be generic, because in another situation wrong parameters could be expected and using exception would be inappropriate.

 

What do you recommend? Maybe there is some pattern for such situations?

[smoseley]

I wouldn't suggest either of those.  I would do something like:

 

try {
    if (checkUserAndKey()) {
        // do stuff
    }
} catch (Exception $e) {
    echo $e->getMessage();
}

 

checkUserAndKey() can throw an exception if an error is found, or return true if it succeeds.

[Mahngiel]

What is the controller / script action like that sends data to  GetPageContent() and what requests it?

[Anonim250]

smoseley,

1) How is your code better than my option 2?

2) Why do you need to return boolean (or anything at all) from the checkUserAndKey() function if it either succeeds or throws an exception?

[Anonim250]

What is the controller / script action like that sends data to  GetPageContent() and what requests it?

 

Well, it's something like this (I believe it's called a page controller, but I'm not sure, because I'm not a web programmer):

 

<?php

require_once 'bootstrap.php'; // init configuration, database connection, autoload, etc.

function GetPageContent()
{
  ...
}

$pageVars = array();
$pageVars['CONTENT'] = GetPageContent();
...
PageRenderer::RenderPage($pageVars); // fills main template and outputs it

?>

[xyph]

PageRenderer::RenderPage seems kind of repetitive heh.

 

Why check for exceptions in the function? Why not just surround your base-code in try/catches, unless your exceptions aren't fatal errors.

[Anonim250]

PageRenderer::RenderPage seems kind of repetitive heh.

 

Why check for exceptions in the function? Why not just surround your base-code in try/catches, unless your exceptions aren't fatal errors.

 

I've just made that name up

Well, in this case - yes, maybe just throwing an exception and logging+showing it in the uncaught exception handler or at the highest level (a front or page controller) is a right way.

But what would you do if an error was expected (so it would not an exceptional situation to throw an exception in), for example checking that the user was not already processed and displaying a nice-looking error if it was.

[xyph]

If you want to use exceptions for non-fatal errors, you have to plan for this in your initial application design. You have to figure out failure points, and how your final output will receive/handle this exception. You definitely want to build different exception classes for each 'type' of error you plan on handling.

 

Keep in mind, having nested try/catch blocks can get tricky. Be sure to document your methods/classes/functions to remind you they're throwing exceptions on error.

[Anonim250]

If you want to use exceptions for non-fatal errors, you have to plan for this in your initial application design. You have to figure out failure points, and how your final output will receive/handle this exception. You definitely want to build different exception classes for each 'type' of error you plan on handling.

 

Keep in mind, having nested try/catch blocks can get tricky. Be sure to document your methods/classes/functions to remind you they're throwing exceptions on error.

 

No, I think I don't want to use exceptions for non-fatal errors. Here is an example:

User gets an activation email with an activation links. Clicks a link and activates his account. But he may click that link second time and we should show him an error that his account was already activated earlier. I don't think this is an exceptional situation, I think this is an expected situation that must be handled differently. How would you make such check? Maybe my option number 1 (see the starting post) or maybe you know a better way?

[xyph]

If an exception will always cause a fatal error, then you only need try/catch blocks at the highest level.

 

Throw an exception if the user posts an invalid (in this case, already used) activation code. Fatal error doesn't mean unexpected (all errors should be 'expected' once your application is in a production state), it simply means if an error occurs, there's no point in executing anything beyond the code required to display the error correctly

[Anonim250]

So, as I understand you would still throw an exception, catch it on a highest level and show it nicely?

[xyph]

It's hard to say exactly what I'd do without sitting down and going over every aspect of your application's design and purpose, but that's generally what I use exceptions for.

 

Catch a fatal error, stop the rest of the module from executing, and display the error using the current template.

[smoseley]

Exceptions are exceptional.    I suggest using them everywhere!