Re: error code from delete or beforeDelete?

On Wednesday, 30 May 2012 13:00:47 UTC+2, Ratty wrote:

Thanks for this thread actually Bill, it has given me a lot of pause for thought...

Exceptions are a method of reporting WHAT has gone wrong or is about to go wrong. It is not a method of reporting WHERE it went wrong. In my experience they are primarily used for detecting "This should not happen" conditions. In your example the Category model delete function should not be called unless that category is empty, so an exception could be thrown if that condition is not met. This is a long way away from just calling delete() and see if it works. Remember that an exception can be thrown in beforeDelete(), delete(), afterDelete() in your model, any parent model, or any related model that is affected in HABTM, HasMany, ... relationships and they will all arrive at your catch block. You could end up with code like...

try {
   $this->Category->delete( $id );
} catch( NotEmptyCategoryException $nece ) {
     ....
} catch( CategoryHasChildCategoriesException $ccce ) {
    ....
} catch( CategoryReferencedInBlogTableException $crbe ) {
    ....
} catch( ChildCategoryOfThisCategoryNotEmptyException $damn ) {
    ....
}

This logic, to my mind, would be far better positioned in the model function...

Category::isDeletable($id) {
    $result = true;
    if( $this->hasItems() ) {
       $this->log( 'Cannot delete category containing items.', 'debug' );
       $result = false;
    }
    if( $this->hasChildCategories() ) {
        $this->log('Cannot delete category with child categories.', 'debug' );
        $result = false;
    }
    ...
    return $result;
}

CategoriesController::delete( $id ) {
    try {
        if( $this->Category->isDeletable($id) ) {
            $this->Category->delete($id);
        }
    }
    catch( Exception $e ){
        // If isDeletable has done it's job, this should never happen.
        $this->log( $e->message(), 'error' );
        ....
    }
}

Not only is this cleaner, but the model is FAR easier to write test cases for.

In what way is that FAR easier to test.

Note you've changed:

    $this->Category->delete( $id );

into

    if ($this->Category->isDeletable($id)) {

        $this->Category->delete( $id );

    }

Which means you're either checking things twice - or delete is nolonger atomic.

AD

--
Our newest site for the community: CakePHP Video Tutorials http://tv.cakephp.org
Check out the new CakePHP Questions site http://ask.cakephp.org and help others with their CakePHP related questions.
 
 
To unsubscribe from this group, send email to
cake-php+unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/cake-php