8
\$\begingroup\$

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?

\$\endgroup\$
6
  • \$\begingroup\$ What language is this? Be sure to add its tag to this post. \$\endgroup\$ – Jamal Aug 3 '13 at 2:36
  • \$\begingroup\$ @jamal added to tags \$\endgroup\$ – Jonnny Aug 3 '13 at 2:38
  • 1
    \$\begingroup\$ You can use active record (instead of using DAO like you did), then you can setup validation rules along with the message in the model. \$\endgroup\$ – user28339 Aug 12 '13 at 7:39
  • \$\begingroup\$ Why to you echo your results of the actions? I'm not familiar with your framework, but I guess there is some kind of views where you can assign the success state or error messages and all the layout is handled in a template? \$\endgroup\$ – mheinzerling Aug 12 '13 at 12:49
  • \$\begingroup\$ Are you working with db commands in the controller ? \$\endgroup\$ – Shaz Aug 14 '13 at 18:56
4
\$\begingroup\$
  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;
        ...
    }
    
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.