returning error object or true/false ???

[mr_jeep]

Hi, now I am confused.. very very very confused. I'm wondering if, in a method,
I should return an error object (somewhat similair to Pear_Error) or just return true or false. I looked to some PEAR files and I can't find the right pattern about when returning error object or true/false. Here's an exemple :

    // }}}
    // {{{    open()
    
    /**
     * Ouvre le fichier
     *
     * @param mode int Le mode du fichier
     *
     * @return bool true si le fichier a été ouvert, false sinon
     *
     * @access public
     */
    function open($mode = FILE_OPEN_RW_CREATE)
    {
        $this->fp = @fopen($this->file, $mode);

        return ($this->fp) ? true : false;
    } 

or :

    // }}}
    // {{{    open()
    
    /**
     * Ouvre le fichier
     *
     * @param mode int Le mode du fichier
     *
     * @return mixed true si le fichier a été ouvert sinon un objet error
     *
     * @access public
     */
    function open($mode = FILE_OPEN_RW_CREATE)
    {
        $this->fp = @fopen($this->file, $mode);
        if (!$this->fp) {
            return raiseError(sprintf('Unable to open file %s', $this->file));
        }
        
        return true;
    } 

[Widow Maker]

Personally I would return a boolean, but that is just convienence for me

In saying that, there are certain situations where you are better to return an object though, even I might add, you need to return an object

[David C]

I think code is much easier to follow if you're returning boolean values - I would be shocked if isValid() didn't return true or false.

Also, by separating external classes from each of your classes, you can make your code much more portable.

[McGruff]

I would think it's always better to keep things as simple as possible.

[sweatje]

instead of

function open($mode = FILE_OPEN_RW_CREATE)
    {
        $this->fp = @fopen($this->file, $mode);

        return ($this->fp) ? true : false;
    } 

you could take it all in one shot:

function open($mode = FILE_OPEN_RW_CREATE)
    {
        return ($this->fp = @fopen($this->file, $mode)) ? true : false;
    } 

[Captain Proton]

Boy am I glad that PHP 5 features exceptions

[lazy_yogi]

Yeh I know !!

This proceedural error handling is a nightmare in comparison.

[Betcour]

I personnaly like to standardize on "false" for error, anything else otherwise. This is what built-in PHP functions do most often anyway.

Treating error as objects requires additional work in error checking. What I like to do is have a $error variable in all my object, which contain false if the function worked properly, or an error message otherwise (on top of returning false). That way if I want some detail about the error I can just check the message, and if I don't I just look at the return value.

[lazy_yogi]

OOP Exception handling takes care of exactly that and alot of other problems when dealing with errors.

[coo_t2]

I've gotten so I return an error object in any situation where something failed or something didn't go as expected. I would consider fopen() failing as something going wrong, so I would return an error.

Speaking of PHP5 exceptions. Will PHP's built-in functions throw exceptions?

--ed

[Taoism]

Hi, now I am confused.. very very very confused. I'm wondering if, in a method,
I should return an error object (somewhat similair to Pear_Error) or just return true or false.

I always return an AppError object whenever I encounter an error. For the simple reason that it removes ambiguity.

For example, if you have a function that does a check say, user_exists() - it should return a true if a user exists, or a false if a user doesn't exist. Let's say in this function you query a db for this check. What if the query fails? Not that it returns 0 rows, which should be a false, but that there is a trappable db error? You can't return a false to indicate the error, since false is a valid response of the function [and thus the error should not be taken to mean the user doesn't exist because they might be in the db despite the error]. I would trap the db error, and create, then pass back an AppError object. I haven't abended my code, but I can now handle the error, and if I really want to, I could parse the error and set up different application responses depending on the type of error returned.

Coding for returning errors by booleans leads to "coding by exception" - That is, "a false means error, except when...."

Cheers,
Keith.

[McGruff]

It may be better not to do two things in one function. Kind of blurs the focus a little.

[Taoism]

It may be better not to do two things in one function. Kind of blurs the focus a little.

I think you are missing the point...

Cheers,
Keith.

[lastcraft]

Hi...

Coding for returning errors by booleans leads to "coding by exception" - That is, "a false means error, except when...."

I don't think is solved by returning application objects either.

I would expect methods named as assertions to return true/false, for example userExists(). A problem with returning an error object is that if it has data in it, such as an error message, it will evaluate true. So...

if (! $authenticator->userExists()) {
    ...
} 

...could cause surprises.

I find that methods that fetch information are mor elikely candidates for error objects if they are high up in the application hierarchy. This is a no brainer if you are carrying a message that will be viewed by the user. You still have to check that the response is an error, though, unless using the null value pattern. Checking class names is restrictive so we tend to have both success and failure objects...

class Success() {
    var $_result;

    function Success(&$result) {
        $this->_result = &$result;
    }
    function isError() {
        return false;
    }
    function &getResult() {
        return $this->_result;
    }
}
class Failure() {
    var $_message;

    function Failure($message) {
        $this->_message = $message;
    }
    function getError() {
        return $this->_message;
    }
    function isError() {
        return true;
    }
}
class DatabaseError extends Failure {
    function DatabaseError() {
        $this->Failure('Please contact the system administrator...');
    }
} 

That way the conditional is simpler...

$response = &$authenticator->findUserByLogin();
if ($response->isError()) {
    ...
}
$permissions = $authorisor->getPermissions($response->getResult()); 

To be honest, at the lower levels this is overkill. Just adding an isError() and getError() to the finder (here the authenticator) is sufficient.

yours, Marcus

[Taoism]

Hi...
I don't think is solved by returning application objects either.

I respectfully disagree. It solves the issue completely.

A problem with returning an error object is that if it has data in it, such as an error message, it will evaluate true. So...

if (! $authenticator->userExists()) {
    ...
} 

...could cause surprises.

Which is why I code the AppError object to contain an is_error() method:

//inside AppError
  function is_error($obj){
    return (is_object($obj)&&is_a($obj,'apperror'))?true:false;
  } 

Then I can handle an error check like so....

if(AppError::is_error($result=function_or_object_method_call()){
  //handle error
}
//else clause is optional depending on how
//error is handled 

Problem solved. It works quite well and removes ambiguity.

Cheers,
Keith.

[lastcraft]

Hi...

Problem solved. It works quite well and removes ambiguity.

I respectfully disagree with your respectful disagreement .

Unfortunately by doing a class name check in the AppError you have tied youself to a specific class hierarchy. You have in effect turned PHP into a version of Java, but without interfaces to get you out of trouble. That was why I added the Success wrapper as well - to restore polymorphism.

The hard coding of the class name has big and painful implicatons for other people extending your framework and for testing. Also it makes the is_error() function mysterious. Someone trying to figure out your system basically has to read the code, as it's invisible to the interfaces.

Of course this abomination was originally perpetrated by PEAR and various authors have copied it by example .

yours, Marcus

[Taoism]

Unfortunately by doing a class name check in the AppError you have tied youself to a specific class hierarchy. You have in effect turned PHP into a version of Java, but without interfaces to get you out of trouble.

I am not even sure how to answer this, except to say that while I am not trying to turn "PHP into a version of Java", the intent of the AppError class is exactly as you state - to tie the development to a specific class [that denotes a trapable error].

When I originally wrote the class, I was extending it, somewhat like you showed in your post.

class FooBar extends AppError{} 

But in the end I realized that I wasn't making use of the fact that I had a DataBaseError vs a FileReaderError. In the end, the test came down to, are these of the AppError object in some way? So, I removed the sub-classing. It still exists in a few spots for BC reasons, but will be factored out at some point.

That was why I added the Success wrapper as well - to restore polymorphism.

That is an interesting take, and I am a little embarrassed I never took that step when I first started sub-classing my errors so long ago.

The hard coding of the class name has big and painful implicatons for other people extending your framework and for testing. Also it makes the is_error() function mysterious. Someone trying to figure out your system basically has to read the code, as it's invisible to the interfaces.

To me there is no big or painful implication. If someone at work wants to leverage this code, then they can simply look at the API and find out what the deal is with AppError. Just like they would/should do with any function method they don't understand, whether it is native PHP or a user-created one.

I'm sorry Marcus, but this last point really doesn't make a lot of sense to me. I see the point you are trying to drive at, but to me it doesn't fly. If someone doesn't like the way the framework works, I am not holding a gun to their head to use it. If I don't like PEAR:DB (which I don't), then I don't use it. If I don't like Smarty (which I don't) I don't use it. If I were to write modules for the PostNuke project, should I complain because I don't like the way their API interface is set up? I seriously doubt there is *any* framework where the programmer doesn't need to either a) look at the source code, or b) read an API. And that is all that is entailed by any of the new co-op students that comes through our office every 6 months to do a work term.


Cheers,
Keith.

[lastcraft]

Hi...

Sorry, I wasn't having a go at you. I think a PEAR moan got in there some how.

When I originally wrote the class, I was extending it, somewhat like you showed in your post.

This is fine in the context of an application, where you are already at the top of the pile and control the lower level libraries. It doesn't work well in the context of a library or framework, because the error object may have it's own descendency already. Even if it is given the same interface it would still have to be wrapped.

You can mitigate things somewhat with...

class AppError {
    ...
    function isError($response) {
        if (! method_exists($response, 'isError')) {
            return false;
        }
        return $response->isError();
    }
} 

The other nuisance is that AppError::isError() static calls will litter the place. A change of framework will result in changing all of these in areas unrelated to the domain of the original framework.

To me there is no big or painful implication.

Ok, I exaggerated a bit . I would like to follow the argument through to it's conclusion if I can...

...they can simply look at the API and find out what the deal is with AppError.

The point is that the there is some counter intuitive extra information - that the error must be descended from AppError and that a callback is necessary. To understand what is happening I have to read the API for the actual error subclass, the API for the AppError parent so as to see the comment that all errors must be descended from it. I also have to read is_error() to realise it is a static callback (my IDE won't tell me). If I skim read I'll likely call is_error() directly and will never trap any errors - not a nice bug.

All this to check an error .

If someone doesn't like the way the framework works, I am not holding a gun to their head to use it.

Of course not . There are "frameworks" touted around that should know better.

If I were to write modules for the PostNuke project, should I complain because I don't like the way their API interface is set up?

Yes. If the developer is in the same office, then whining constantly can be very effective.

(Before I get flamed, yes I am joking).

I seriously doubt there is *any* framework where the programmer doesn't need to either a) look at the source code, or b) read an API.

Well no, but you want to keep down the cognitive load. Project specific idioms don't help newcomers, students or otherwise.

Cheers,
Keith.

Apologies for coming on a bit strong, BTW.

yours, Marcus

[been]

I think there might be a significant difference between the methods proposed here by lastcraft and Taoism:

if (AppError::is_error($result = doSomeUsefulStuff($withSomeParameters)) {
     // handle error
 } 

Maybe I'm missing something (please say so if I do), but as much as you can ask: "But what if false is a valid return value?", you can ask: "But what if an AppError is a valid return value?"... Doesn't an analogous problem exist?

Whereas (With the Success and Failure base classes)

$result = doSomeUsefulStuff($withSomeParameters));
 if ($result->isError()) {
     // handle error
 } 

doesn't seem to have this problem: if some sort of error object, or false, or true or whatever, would be a valid return value, $result->isError() would still return false. It would seem to me that the latter method would naturally result in more consistent and more readable code?

With AppError: If an apperror would be a valid return type, it would require a rewrite of the AppError::is_error() code: the return of true or false in the terniary operator statement would have to be the other way around.

Just some thoughts after an initial (and interesting btw) read.
Also, to be clear, it's not my intention to criticize or glorify other people's code, I'm trying to make an objective observation here

[coo_t2]

With AppError: If an apperror would be a valid return type, it would require a rewrite of the AppError::is_error() code: the return of true or false in the terniary operator statement would have to be the other way around.

Why would an AppError ever be a valid return type?
An error should always be treated as an error, shouldn't it?

--ed

[Taoism]

The point is that the there is some counter intuitive extra information - that the error must be descended from AppError and that a callback is necessary. To understand what is happening I have to read the API for the actual error subclass, the API for the AppError parent so as to see the comment that all errors must be descended from it. I also have to read is_error() to realise it is a static callback (my IDE won't tell me).

Just to clarify, the AppError object is no longer subclassed, since there is no need to do this. If an error is to be returned, then an instance of AppError is returned. This simplifies where the programmer needs to look in the API down to one class.

If I skim read I'll likely call is_error() directly and will never trap any errors - not a nice bug.

I wouldn't call it a bug. I would call it a lack of focus for not reading properly

Apologies for coming on a bit strong, BTW.

yours, Marcus

No need to apologise, I greatly enjoy your insights, and quite often learn a new perspective from you. I hope you never took any of my rebuttals as too harsh either. Differences of opinion are what help to create progress

Cheers,
Keith.

[Taoism]

Maybe I'm missing something (please say so if I do), but as much as you can ask: "But what if false is a valid return value?", you can ask: "But what if an AppError is a valid return value?"... Doesn't an analogous problem exist?

With AppError: If an apperror would be a valid return type, it would require a rewrite of the AppError::is_error() code: the return of true or false in the terniary operator statement would have to be the other way around.

You are correct. If AppError could ever be a valid return type, then yes, things could fall down. But, simply, it isn't. In our code, an AppError is always (no exceptions) an error. So your "what if" wouldn't be possible

Cheers,
Keith.

[lastcraft]

Hi...

Just to clarify, the AppError object is no longer subclassed, since there is no need to do this.

Ok, the penny has dropped . You are using a new type as an alternative to true/false/null.

To sum up some alternative approaches...

1) Return an error that does not look like valid data. I'll call these "out of band" errors and this is the normal solution. The AppError type is a way of creating an out of band tunnel at the expense of some loss of portability.
2) Return a true/false response and use a separate method to get the result. The data becomes extra state for the object which will complicate it greatly. I'll call it "sticky data". This is typical of iterators as they already have data state anyway.
3) Use a separate isError() method to see if an error has occoured. I'll call this the "sticky error" method as the error will only be reset on the next real method. This works well for object construction with "new" where no return value is available and a failure results in the object being discarded anyway.
4) Use trigger_error() with an error handler. Something must be done immediately with the error, and so it will probably end up on some kind of Singleton queue. Actually quite portable if you don't inflict your choice of queue object on the client.
5) Create a response wrapper (Success/Failure). Overkill in most situations.
6) Throw an exception (PHP5 only and this is no panacea).
7) Use die(). Rarely acceptable of course, but simple at least.
8) Set a global (or framework wide) error variable. Yuk!

Have I missed any?

My own take on this is to use the appropriate method for each situation. Mix and match is not so much of a problem as the extra code clutter if you select the wrong approach.

No need to apologise, I greatly enjoy your insights, and quite often learn a new perspective from you. I hope you never took any of my rebuttals as too harsh either. Differences of opinion are what help to create progress

Well cheers . Actually I prefer rebuttals. It's the follow on postings that are usually the most interesting.

yours, Marcus