Including a separate method for error reporting

[NotionCommotion]

In the past, I often created methods which would return an empty error array upon success.  I recently had a method which might first validate and then return an object, and I didn't like the idea of having to check if was an object or array to determine whether it was an error.  Thinking instead to (almost) always either return true/false or results/false, and include a method to get the errors.  Is this good or bad practice?  If bad, why?  Thanks

<?php


$callbacks=new CallbackCollections();
if($callback=$callbacks->create('bla')){
    //Success    
}
else {
    exit($callback->getError());
}


class CallbackCollections
{
    private $error=null;


    public function getError()
    {
        return $this->error;
    }


    public function create($class)
    {
        if($mappedClass=$this->class($class)) {
            $this->error=null;
            return new $mappedClass();
        }
        else {
            $this->error="Invalid method $class";
            return false;
        }
    }
    public function destroy($rpcId)
    {
        if(!isset($this->stack[$rpcId])){
            $this->error="not set";
            return false;
        }
        else {
            unset($this->stack[$rpcId]);
            return true;
        }
    }
    public function add($callback)
    {
        $rpcId=++$this->count;
        $this->stack[$rpcId]=$callback;
        return $rpcId;
    }
    public function get($rpcId)
    {
        if(isset($this->stack[$rpcId])) {
            $this->error=false;
            $callback=$this->stack[$rpcId];
        }
        else {
            $callback=false;
            $this->error="Callback does not exist";
        }
        return $callback;
    }
}

[requinix]

I personally don't care for setting a class property for this: it suggests the error is part of the object state when (in this case) it isn't.

 

Some people don't like it but I use references:

public function get($rpcId, &$errors = []) {

Returns true/false and $errors is set if there are any.

[NotionCommotion]

Thanks,  I will think it over and figure out what works for me and try to stay consistent.

[requinix]

Ah. I should have looked at the code closer.

 

There's also exceptions. If the method is supposed to work but you're coding for a situation where it can't, exceptions are appropriate. I think that applies here: given an $rpcId there should be callbacks (or at least the ID should be known even if there aren't any callbacks associated yet) so calling with a bad ID is exception-al behavior.

 

As for the question itself, it's generally not good form for a function to have multiple types of return values except for using null/false for a "failure" value, being a common PHP paradigm. Here I think returning array or null/false (pick consistently) is fine.

$callbacks = $object->get($id);
if ($callbacks !== null) {

[NotionCommotion]

I first was afraid of exceptions, then I loved them, and now I am just not sure.  If the rpcId isn't there, then the user (albeit a machine) provided faulty user data.  Maybe mistakes from some users are exceptional enough?

 

PS.  I typically just say an empty array implies successes.

[requinix]

Exceptions should be an all-or-nothing decision: either use them in all places where it makes sense to, or don't use them (unless it's particularly useful to). So that's a big decision to make.

 

Yet another answer is to not raise an error at all here. Controversial, but I wouldn't mind doing it. If you don't expect it to ever happen and if it does it's not a particularly big problem then it's not so bad.

[NotionCommotion]

Do you use them?  Seems to be some strong opinions on the matter.

 

Some of the errors will just be internally logged, and not communicated back.

[requinix]

I want to but normally don't use them. My code typically only has one or maybe two ways any particular piece can fail "normally" so returning failure is enough. If I did exceptions I would really do exceptions, with inheritance hierarchies and all sorts of details and it's easy for me to get sidetracked down that rabbit hole.

 

The main difficulty I have is the documentation: it's easy to say "function A calls function B so anything unhandled that B @throws should be documented for A too", but adding an exception to B later means tracking down A and everything else that uses it, evaluating how they should react, and probably documenting that this new exception is unhandled too. It's exponential. Don't get me wrong, that would be a good thing to do, but it's so tedious and boring.

 

Anyway, logging might be the best idea here. Return generic failure and log the event. With a good logging system it's easy to just pop in a call to a logger, and in fact that's what I do. To me, the main purpose of exceptions is that they unravel the stack, however I think the real importance is to get a message somewhere noticeable so the bug can be fixed and then let the system recover gracefully - and checking true/false is much easier than listing out all the exceptions that can be caught (and catch-alls are almost always a bad workaround to that).