Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I was recently asked to write a Question-and-Answer feature for my company's product pages using Yii 1. It was my first Yii project, and it was a little rough around the edges (Code A). However, when I submitted my code, my boss re-wrote my two actions to a single action (Code B).

Both Code A and Code B work, but, in general, which one is better and why?

Code A:

/**
 * Creates a new question (and answer if given)
 *
 * @access  public
 * @throws  CDbException
 * @return  void
 *
 */
public function actionCreate()
{
    // create form model
    $model = new QuestionForm('create');

    // if the form was submitted
    if (isset($_POST['QuestionForm'])) {
        // set the attributes
        $model->attributes = $_POST['QuestionForm'];
        // if the model is valid
        if ($model->validate()) {
            // create a new question
            $question = new CustomerQuestion();
            $question->employee_id = Yii::app()->user->id;
            $question->question    = $model->question;
            $question->pseudonym   = $model->questionPseudonym ?: null;
            $question->type        = $model->questionType;
            $question->status      = $model->questionStatus;
            // if the question is saved ok
            $isOk = $question->save();
            if ($isOk) {
                // create a new answer
                $answer = new CustomerAnswer();
                $answer->author_employee_id    = Yii::app()->user->id;
                $answer->customer_question_id  = $question->id;
                $answer->answer                = $model->answer;
                $answer->pseudonym_employee_id = $model->answerPseudonymEmployeeId ?: null;
                // if the answer is saved ok
                $isOk = $answer->save();
                if ($isOk) {
                    // loop through each item id
                    $insertItemIds = $model->getAuthenticInsertItemIds();
                    foreach ($insertItemIds as $itemId) {
                        // create a new question item
                        $questionItem = new CustomerQuestionItem();
                        $questionItem->item_id              = $itemId;
                        $questionItem->customer_question_id = $question->id;
                        // if an error occurs, stop
                        $isOk = $questionItem->save();
                        if ( ! $isOk) {
                            break;
                        }
                    }
                    // if great success!
                    if ($isOk) {
                        // flash the good news and redirect to question/index
                        Yii::app()->user->setFlash(
                            'success', 
                            'Your question was created successfully'
                        );
                        $this->redirect($this->createUrl('question/index'));
                    } else {
                        $model->addErrors($questionItem->getErrors());
                    }
                } else {
                    $model->addErrors($answer->getErrors());
                }
            } else {
                $model->addErrors($question->getErrors());
            }
        }
    }

    // get all the employees
    $employees = Employee::model()
        ->scpIsActive()
        ->scpOrderFirstName()
        ->findAll();

    // create the view
    $this->render('create', [
        'employees' => $employees,
        'model'     => $model
    ]);

    return;
}

/**
 * Updates an existing question
 *
 * @access  public
 * @return
 *
 */
public function actionUpdate($questionId)
{
    // if the question id is valid
    if (is_numeric($questionId) && is_int(+$questionId) && $questionId > 0) {
        // if the question is authentic
        $question = CustomerQuestion::model()->findByPk($questionId);
        if ( ! empty($question)) {
            // create a new form model
            $model = new QuestionForm('update');
            // get the question's "old" item ids
            $oldItemIds = [];
            foreach ($question->Items as $item) {
                $oldItemIds[] = $item->id;
            }
            // set the starting value of the model's attributes
            $model->question       = $question->question;
            $model->questionType   = $question->type;
            $model->questionStatus = $question->status;
            $model->itemIds        = implode(', ', $oldItemIds);
            // get the question's first answer...
            //
            // a question can have multiple answers
            // however, we only support a single answer for now
            // it's a simpler workflow, and our staff answers are definitive
            // if a question doesn't have an answer, $answer will not be set
            //
            foreach ($question->CustomerAnswers as $answer) {
                break;
            }
            if (isset($answer)) {
                $model->answer                    = $answer->answer;
                $model->answerPseudonymEmployeeId = $answer->pseudonym_employee_id;
            }               
            // if the form was submitted
            if (isset($_POST['QuestionForm'])) {
                // create the form model
                $model->attributes = $_POST['QuestionForm'];
                // if the form is valid
                if ($model->validate()) {
                    // save the question's old status
                    $oldStatus = $question->status;
                    // update the question's attributes
                    $question->question  = $model->question;
                    $question->pseudonym = $model->questionPseudonym ?: null;
                    $question->type      = $model->questionType;
                    $question->status    = $model->questionStatus;
                    // if the question is saved ok
                    $isOk = $question->save();
                    if ($isOk) {
                        // determine what to do with the answer...
                        //
                        // how we handle the answer depends on several factors
                        // if the question had an answer and has an answer, update it
                        // if the question didn't have an answer and does now, insert it
                        // if the question had an answer and doesn't now, raise an error
                        // too much has happened with an answer to delete it
                        // 
                        $hadAnswer = ! empty($answer);
                        $hasAnswer = ! empty($model->answer);
                        if ($hadAnswer && $hasAnswer) {
                            // update the existing answer
                            $answer->answer                = $model->answer;
                            $answer->pseudonym_employee_id = $model->answerPseudonymEmployeeId ?: null;
                            $isOk = $answer->save();
                        } elseif ($hasAnswer) {
                            // create a new answer
                            $answer = new CustomerAnswer();
                            $answer->customer_question_id  = $question->id;
                            $answer->author_employee_id    = Yii::app()->user->id;
                            $answer->answer                = $model->answer;
                            $answer->pseudonym_employee_id = $model->answerPseudonymEmployeeId ?: null;
                            $isOk = $answer->save();
                        } elseif ($hadAnswer) {
                            // try to update the existing answer
                            // this will raise a validation error (and that's ok)
                            $answer->answer                = null;
                            $answer->pseudonym_employee_id = $model->answerPseudonymEmployeeId ?: null;
                            $isOk = $answer->save();
                        } else {
                            $answer = null;
                        }
                        // if whatever we did (or didn't do) to the answer is ok
                        if ($isOk) {
                            // get the items to delete
                            $deleteItemIds = $model->getDeleteItemIds($oldItemIds);
                            foreach ($deleteItemIds as $itemId) {
                                // find the question-item
                                $questionItem = CustomerQuestionItem::model()
                                    ->findByAttributes([
                                        'item_id'              => $itemId, 
                                        'customer_question_id' => $question->id
                                    ]);
                                // if the question-item is not deleted ok, break
                                $isOk = $questionItem->delete();
                                if ( ! $isOk) {
                                    break;
                                }
                            }
                            // if there were no deletes or they were all successfull
                            if ($isOk) {
                                // get the items to insert
                                $insertItemIds = $model->getAuthenticInsertItemIds($oldItemIds);
                                foreach ($insertItemIds as $itemId) {
                                    // create a new question-item
                                    $questionItem = new CustomerQuestionItem();
                                    $questionItem->item_id              = $itemId;
                                    $questionItem->customer_question_id = $question->id;
                                    // if the question-item is not saved ok, break
                                    if ( ! $questionItem->save()) {
                                        break;
                                    }
                                }
                                // if ok
                                if ($isOk) {
                                    // if the question's status changed
                                    if ($question->status != $oldStatus) {
                                        // if the question was asked by a user
                                        // (i.e., don't send notifications to employees)
                                        //
                                        if ( ! empty($question->user_id)) {
                                            // send an email notification
                                            $this->sendMail($question, $answer);
                                        }
                                    }
                                    // great success!
                                    // flash the user and redirect to question/index
                                    //
                                    Yii::app()->user->setFlash(
                                        'success', 
                                        'Your question was updated successfully'
                                    );
                                    $this->redirect($this->createUrl('question/index'));
                                } else {
                                    $model->addErrors($questionItem->getErrors());
                                }
                            } else {
                                $model->addErrors($questionItem->getErrors());
                            }
                        } else {
                            $model->addErrors($answer->getErrors());
                        }
                    } else {
                        $model->addErrors($question->getErrors());
                    }
                }
            }

            // get all the employees
            $employees = Employee::model()
                ->scpIsActive()
                ->scpOrderFirstName()
                ->findAll();

            // create the view
            $this->render('update', [
                'model'     => $model,
                'employees' => $employees
            ]);

        } else {
            throw new CHttpException(
                404, "A question with id '$questionId' does not exist"
            );
        }
    } else {
        throw new CHttpException(
            400, "A question id parameter is required"
        );
    }

    return;
}

Code B:

/**
 * Creates a new question (and answer if given)
 *
 * @access  public
 * @throws  CDbException
 * @return  void
 *
 */
public function actionCreate()
{
    $this->prepareAndSave( 'insert' );
}

/**
 * Updates an existing question
 *
 * @access  public
 * @return
 *
 */
public function actionUpdate($questionId)
{
    $this->prepareAndSave( 'update', CustomerQuestion::model()->findByPk( $questionId ) );
}

private function prepareAndSave( $type, $question=null )
{
    if ( ( $type !== 'insert' ) && ( $type !== 'update' ) )
        Throw New CHttpException( 400, "[Internal error] Unknown type sent to " . __FUNCTION__ . " in " . __CLASS__ . ": " . $type );

    $formModel = New QuestionFormModel( $type );
    $formModel->setQuestionId( $question ? $question->id : null );
    // If we try to load data into the formModel, but there is none to be found
    //   if this is an update and has a valid question, that information needs to be added to the formModel
    if ( $formModel->loadData() === false )
        $formModel->loadDataFromAR( $question );
    // If we are here, then the loadData() succeeded and data was sent that the formModel could use.
    //   Now we need to make sure the data is valid.  If it is valid we'll save it to the database.
    elseif ( $formModel->validate() === true )
    {
        // Everything should be good, time to save it to the database

        $questionStatusChanged = ( $question->status !== $formModel->questionStatus );

        // Prep and validate question
        if ( $question === null )
        {
            $question = New CustomerQuestion();
            $question->employee_id = Yii::user()->isGuest() ? 7 : Yii::user()->id;
        }
        $question->pseudonym = $formModel->questionPseudonym ?: null;
        $question->status    = $formModel->questionStatus;
        $question->type      = $formModel->questionType;
        $question->question  = $formModel->question;

        $questionValid = $question->validate();


        // Prep and validate answer
        $answer = null;
        $answerValid = false;
        if ( $questionValid && $formModel->hasAnswer() )
        {
            // Try to get answer model from existing question
            if ( $question->isNewRecord === false )
                $answer = $question->CustomerAnswer;

            $answerValidateList = null;

            // If no answer model exists, create a new one and load it with starting data.
            if ( $answer === null )
            {
                $answer = New CustomerAnswer();
                // ignore validating the customer_question_id initially if it is not yet available
                $answerValidateList = array_keys( $answer->attributes );
                if ( $question->isNewRecord )
                    $answerValidateList = array_diff( $answerValidateList, [ 'customer_question_id' ] );
                else
                    $answer->customer_question_id = $question->id;
                $answer->author_employee_id = $formModel->answerAuthorEmployeeId;
            }

            $answer->pseudonym_employee_id = $formModel->answerPseudonymEmployeeId ?: null;
            $answer->answer = $formModel->answer;

            $answerValid = $answer->validate( $answerValidateList );
        }

        // Question is valid.  Save question and questionItem.
        if ( $questionValid )
        {
            $question->save( false );

            $questionItemIdsToAdd = $formModel->itemIdList;
            foreach( $question->CustomerQuestionItems AS $qi )
            {
                // if an item id found in the database is in the add list, then remove it from the add list
                if ( in_array( $qi->item_id, $questionItemIdsToAdd ) )
                    $questionItemIdsToAdd = array_diff( $questionItemIdsToAdd, [ $qi->item_id ] );
                // if an item id found in the database is not in the add list, then remove it from the database
                else
                    $qi->delete();
            }
            // Everything left in the add list needs to be added to the database
            foreach( $questionItemIdsToAdd AS $itemId )
            {
                $qi = New CustomerQuestionItem();
                $qi->customer_question_id = $question->id;
                $qi->item_id = $itemId;
                $qi->save();
            }
        }

        // Answer is valid.  Save answer.
        if ( $answerValid )
        {
            $answer->customer_question_id = $question->id;
            $answer->save( false );
        }

        if ( ! $questionValid || ( $formModel->hasAnswer() && ! $answerValid ) )
        {
            $failureList = [];
            if ( ! $questionValid )
                $failureList[] = 'question';
            if ( ! $answerValid )
                $failureList[] = 'answer';
            Yii::app()->user->setFlash(
                'failure', 
                'An unknown error occured saving your ' . implode( ' and ', $failureList ) . '.  Please contact IT with the following information: '
                        . ( $question ? CHtml::errorSummary( $question ) : 'No problems with question.  ' )
                        . ( $answer ? CHtml::errorSummary( $answer ) : 'No problems with answer.  ' )
            );
        }
        else
        {
            if ( $questionStatusChanged )
                $this->sendMail( $question, $answer );
            Yii::app()->user->setFlash(
                'success',
                'Question ' . ( $answerValid ? 'and answer' : '' ) . ' updated successfully.'
            );
        }

        // Redirect to the update page for this question
        if ( $question->id )
            $this->redirect( [ 'question/update', 'questionId' => $question->id ] );
        else
            $this->redirect( [ 'question/create' ] );
        Yii::app()->end();
    }

    // This could be displayed if there is no data sent or if validation fails
    $this->render( 'edit', [
        'model'     => $formModel,
    ] );
}
share|improve this question
    
In your code if the business logic changes then you have to do the change in two places and then you can make a mistake, in your boss code the logic is in one place so maintenance should be easier. A better explain: en.wikipedia.org/wiki/Don't_repeat_yourself –  Aguardientico Aug 16 at 3:36
2  
@Aguardientico Please answer as an answer, not as a comment. –  200_success Aug 16 at 4:40
3  
Given that you wrote this for your company, are you sure you're allowed to post it here? –  Chris Hayes Aug 16 at 8:13
    
@ChrisHayes That's not entirely our concern. If his company wants it down, they'll let us know. But this is code that's intended to be used as a public example code of their product. "I was recently asked to write a Question-and-Answer feature for my company's product pages using Yii" –  nhgrif Aug 16 at 13:30
1  
@nhgrif I'm not asking from a position of concern for StackExchange but for the asker. I also don't get the impression that the feature involves the source code being available - not sure where you're seeing that? –  Chris Hayes Aug 16 at 18:24

2 Answers 2

In addition to the removed duplicated code, the second version has other improvements too.

Instead of deeply nested if ($isOk) { ... } blocks, it has flattened the logic which is a lot more readable. And instead of $isOK which doesn't give a clue what it's about, he uses more meaningful names like $questionValid and $answerValid.

Note that this kind of repeated evaluation is well justified:

if ( $questionValid ) {
    // ...
}
if ( $answerValid ) {
    // ...
}
if ( ! $questionValid || ( $formModel->hasAnswer() && ! $answerValid ) ) {
    // ...
}

You might try to rewrite this with nested conditions to avoid the repeated evaluation of $questionValid and ! $questionValid, but the performance gain would be insignificant, and you would sacrifice readability.

I would recommend to go one step further, and decompose prepareAndSave to multiple functions. It's doing too many things:

  • prepare question
  • save question
  • prepare answer
  • save answer

These are different responsibilities, it would be good to extract them into independent functions, each responsible for one thing and one thing alone.

share|improve this answer
    
I agree with removing the duplicated code part, and I should have done a better job of that. However, I usually try to err on the side of KISS (aka, keep it stupid simple) over DRY (aka, don't repeat yourself). –  Jack Aug 16 at 17:30
    
Oh, and see, call me crazy, but I like nested if statements. They're like a checklist of tests that must be passed for the action to occur - kind of like the "One True Path". Is that a bad idea? –  Jack Aug 16 at 17:33
    
Deeply nested constructs are generally considered a bad idea, with few exceptions. And it's better to err on DRY than KISS. Simple is easier to refactor. –  janos Aug 16 at 17:51

In your code if the business logic changes then you have to do the change in two places and then you can make a mistake.

In your boss code the logic is in one place so maintenance should be easier.

A better explain: http://en.wikipedia.org/wiki/Don't_repeat_yourself

share|improve this answer
    
thanks for the answer. I totally agree with the DRY principle (and you), but to me, his code seemed harder to follow - i.e., the maintenance benefit gained in DRY was lost in KISS (keep-it-stupid-simple). –  Jack Aug 16 at 17:16

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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