PHP multiple if / elseif and error messaging / handling best practices

Question

This is a problem I come accross fairly regularly and I've never found / figured out a best practices situation. Exceptions are probably the way to go however the application I'm working makes no use of them so I'm trying to stick to the currently used methods.

What is the best way to lay out if statements, returns, messages etc. in the event that 3, 4, 5 or more different conditions are required to be checked and either an error message is set or the processing continues. Is it best practice to have all error checking physically at the start of the code?

Here's an example with some real-world type conditions.

function process($objectId,$userId,$newData)
{
    $error = '';
    if(($object = $this->getObject($objectId)) && $object->userOwnsObject($userId))
    {
        if($this->isValid($newData))
        {
            if($object->isWriteable())
            {
                if($object->write($newData))
                {
                    // No error. Success!
                }
                else
                {
                    $error = 'Unable to write object';
                }
            }
            else
            {
                $error = 'Object not writeable';
            }
        }
        else
        {
            $error = 'Data invalid';
        }
    }
    else
    {
        $error = 'Object invalid';
    }
    return $error;
}

OR

function process($objectId,$userId,$newData)
{
    $error = '';
    if((!$object = $this->getObject($objectId)) && !$object->userOwnsObject($userId))
    {
        $error = 'Object invalid';
    }
    elseif(!$this->isValid($newData))
    {
        $error = 'Data invalid';
    }
    elseif(!$object->isWriteable())
    {
         $error = 'Object not writeable';
    }
    elseif(!$object->write($newData))
    {
        $error = 'Unable to write to object';
    }
    else
    {
        // Success!
    }
    return $error;
}

It's clear to me that in this case option 2 is the way to go. It's much clearer. Now, we can make it a bit more complex:

function process($objectId,$userId,$newData)
{
    $error = '';
    if(($object = $this->getObject($objectId)) && $object->userOwnsObject($userId))
    {
        $this->setValidationRules();
        $parent = $object->getParentObject();
        $parent->prepareForChildUpdate();

        if($this->isValid($newData,$parent))
        {
            $newData = $this->preProcessData($newData);

            if($object->isWriteable())
            {
                // doServerIntensiveProcess() has no return value and must be done between these two steps
                $this->doServerIntensiveProcess();

                if($object->write($newData))
                {
                    // No error. Success!
                    $parent->childUpdated();
                }
                else
                {
                    $error = 'Unable to write object';
                }
            }
            else
            {
                $error = 'Object not writeable';
            }
        }
        else
        {
            $error = 'Data invalid';
        }
    }
    else
    {
        $error = 'Object invalid';
    }
    return $error;
}

OR this which has some issues with it

function process($objectId,$userId,$newData)
{
    $error = '';
    if((!$object = $this->getObject($objectId)) && !$object->userOwnsObject($userId))
    {
        $error = 'Object invalid';
    }
    // Is it wrong to hate multi-line conditionals?
    elseif(!$this->setValidationRules() || (!$parent = $object->getParentObject()) ||
        !$parent->prepareForChildUpdate() || !$this->isValid($newData,$parent))
    {
        $error = 'Data invalid';
    }
    elseif((!$newData = $this->preProcessData($newData)) || !$object->isWriteable())
    {
         $error = 'Object not writeable';
    }
    // Where does doServerIntensiveProcess() with no return value go??
    elseif(!$object->write($newData))
    {
        $error = 'Unable to write to object';
    }
    else
    {
        // Success!
         $parent->childUpdated();
    }
    return $error;
}

I'm just not sure of the best way to handle this nested if-this-then-do-that-then-if-this-then-do-that kind of functionality. Thank you indvance for any insight you can provide!

Answer 1

What I tend to do to keep code clean is like so:

function process($objectId,$userId,$newData)
{
    $object = $this->getObject($objectId);

    if($object === false)
    {
        return "message";
    }

    if($object->userOwnsObject($userId) === false)
    {
        return "message";
    }

    if($this->setValidationRules() === false)
    {
        return "unable to set validation rules";
    }

    if(false !== ($parent = $object->getParentObject()))
    {
        return "unable to get parent object";
    }

    /*... etc ...*/

    //if your here the all the checks above passed.
}

by doing it this way your also saving resources as your your directly returning in place, the code looks cleaner and no need for 2 many nests

but if your building the function from scratch I don't see why you cant use exceptions in in your new code, it will not interfere with the current app, and makes live simpler

function process($objectId,$userId,$newData)
{    
    if(false !== ($parent = $object->getParentObject()))
    {
          throw Exception("unable to get parent object");
    }

    /*... etc ...*/
}

and

try
{
    $this->process(....);
}
catch(Exception $e)
{
    show_error_page('invalid.php',$e);
}

or another way is to create an error handling class with a static method called InternalError like so

abstract class Error
{
    public static InternalError(Exception $Ex)
    {
        Logger::LogException($Ex);
        //Then flush all buffers and show internal error,
    }
}

so instead of of the show_error_page above you can do:

try
{
    $this->process(....);
}
catch(Exception $e)
{
    Error::InternalError($e); //this provides user with an interface to report the error that has just been logged.
}

this way all your Exception(s) are logged and can be viewed within your administration system, meaning you can track errors faster and not rely on members to visibly see errors, but get a nice apology with an email form asking them to describe what they was trying to do, the error ID will be attached to the form, so you can trace the user to the error.

That's the best form of error handling IMO.

Answer 2

I don't think it is wrong per se to write multi-line conditionals, but you can make it more readable by putting conditions into a variable and use that in your if-statment. I think the elseif constuct is better.

Answer 3

I would completely omit

if($object->isWriteable())
{
    if($object->write($newData))
    {
        // ..
    }
}

And throw Exceptions when calling object-write() (without a return value) instead. Same for the other examples

if ($something->isValid($data)) {
    $op->doSomething($data); // throws XyException on error
}

If you really want to use such constructs, you can also use the swtch-Statement

switch (true) {
    case !$something->isValid($data):
        $errors = "error";
        break;
    // and so an
}

But I really recommend Exceptions.

Answer 4

What about this?

function process($objectId,$userId,$newData)
{
    if((!$object = $this->getObject($objectId)) && !$object->userOwnsObject($userId))
        return 'Object invalid';
    elseif(!$this->isValid($newData))
        return 'Data invalid';
    elseif(!$object->isWriteable())
        return 'Object not writeable';
    elseif(!$object->write($newData))
        return 'Unable to write to object';

    // Success!
}

For the more complex example::

function process($objectId,$userId,$newData)
{
    if(!($object = $this->getObject($objectId)) || !$object->userOwnsObject($userId))
        return 'Object invalid';

    $this->setValidationRules();
    $parent = $object->getParentObject();
    $parent->prepareForChildUpdate();

    if(!$this->isValid($newData,$parent))
        return 'Data Invalid';

    $newData = $this->preProcessData($newData);

    if(!$object->isWriteable())
        return 'Object not writable';

    // doServerIntensiveProcess() has no return value and must be done between these two steps
    $this->doServerIntensiveProcess();

    if(!$object->write($newData))
        return 'Unable to write object';

    // No error. Success!
    $parent->childUpdated();
    return '';
}