Yii exception usage

Question

I've never used a framework before, so I wanted to see if this fairly simple scenario was done correctly or could be improved:

public function actionCreate($id) {

    // Is request Ajax
    if(Yii::app()->request->getIsAjaxRequest()) {

    $userID = Yii::app()->user->id;

        try {

        $cmd = Yii::app()->db->createCommand();
        $cmd->insert('potential_item',array(
        'item_id'=> (int) $id,
        'user_id' => (int) $userID,
        ),'id=:id', array(':id'=>$id));

            echo 'Item Added';

        } catch(CDbException $e){

            echo '<strong>Error!</strong> ' .  $e->getCode() . '</div>'; 
        }
    }
}

Here's what I wanted to achieve with the code. It's basically just a wish list. It's a pretty basic SQL builder statement that I think is secure.

I have a constraint on the DB so that a user and item ID have to be unique. That's why I used the try catch block, and it's really the only DB issue I see occurring. Would the scenario I have used work okay, or am I missing other exceptions?

I also decided to echo the error code. Would you recommend that I just leave a generic message for the end user?

As I really only expect a constraint error, should I check the error code with an if-statement?

Answer 1

1. The code closes a div tag here:

   echo '<strong>Error!</strong> ' .  $e->getCode() . '</div>'; 
   

   But there isn't any opening div tag inside the function. I guess it's a bug.

2. The name of the function (actionCreate) suggests that it creates an action but it also prints to the output. It's misleading. See also: Is it bad practice to output from within a function?

3. I also decided to echo the error code. Would you recommend that I just leave a generic message for the end user?

   I don't think that any of your users will be happy when they see that error code. It's probably not useful for them (except they also develop the application). Error messages should give a suggestion about what the user should do. (See: Should we avoid negative words when writing error messages?, What will be the Best notifications and error messages?)

   If it's not their fault say that. I like the way that Stack Overflow handles these errors:

   Oops! Something Bad Happened!

   We apologize for any inconvenience, but an unexpected error occurred while you were browsing our site.

   It’s not you, it’s us. This is our fault.

   [...]

   You should also log the errors to a log file. Otherwise you will never know that your users has problems.

4. As I really only expect a constraint error, should I check the error code with an if-statement?

   If that's a normal usage (and not a programming mistake) that you have these errors and your database don't check it I'd check it. Otherwise I wouldn't do that.

5. public function actionCreate($id) {
   
       // Is request Ajax
       if(Yii::app()->request->getIsAjaxRequest()) {
   
       $userID = Yii::app()->user->id;
   
           try {
   
           $cmd = Yii::app()->db->createCommand();
           $cmd->insert('potential_item',array(
           $cmd = Yii::app()->db->createCommand();
           $cmd->insert('potential_item',array(
           'item_id'=> (int) $id,
           'user_id' => (int) $userID,
           ),'id=:id', array(':id'=>$id));
   ...
   

   Indentation could be better. Currently it doesn't look like that the $userID = Yii::app()->user->id; is inside the if and $cmd = Yii::app()->db->createCommand(); (and other statements below that) is inside the try block. Consider this:

   public function actionCreate($id) {
   
       // Is request Ajax
       if(Yii::app()->request->getIsAjaxRequest()) {
           $userID = Yii::app()->user->id;
   
           try {
               $cmd = Yii::app()->db->createCommand();
               $cmd->insert('potential_item',array(
                   'item_id'=> (int) $id,
                   'user_id' => (int) $userID,
               ),'id=:id', array(':id'=>$id));
   ...
   

6. I'd rename $id to itemId to reflects its purpose.

7. You could create a local variable for Yii::app().

8. // Is request Ajax
   if(Yii::app()->request->getIsAjaxRequest()) {
   

   The code describes the same, the comment looks redundant for me. (Clean Code by Robert C. Martin: Chapter 4: Comments, Noise Comments)

9. It's hard to figure out the parameters here:

   $cmd->insert('potential_item',array(
   'item_id'=> (int) $id,
   'user_id' => (int) $userID,
   ),'id=:id', array(':id'=>$id));
   

   What is the first, what's the second? Where does the second parameter start? What's its purpose? A few explanatory variable would make it readable:

   $table = 'potential_item';
   $columns = array(
       'item_id' => (int) $id,
       'user_id' => (int) $userID, 
   )
   $cmd->insert($table,$columns, 'id=:id', array(':id'=>$id));
   

   I would have created such variable for the last two parameters two but I've not found any clue about that in the API documentation.

   (Clean Code by Robert C. Martin, G19: Use Explanatory Variables; Refactoring: Improving the Design of Existing Code by Martin Fowler, Introduce Explaining Variable)

10. A guard clause could make the code flatten:

    public function actionCreate($id) {
        if(!Yii::app()->request->getIsAjaxRequest()) {
            return;
        }
        $userID = Yii::app()->user->id;
        ...
    }