Comparison of PHP/Yii controllers

Question

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 rewrote 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,
 ] );
}

• Muhammad Omer Aslam: For the improvements in code B i would prefer to have a cleaner layer of the controller and keep the model layer thick rather than writing all the model related code in the controller. There are still a lot of sections that could be moved into the model layer and shorten the code in the controller. i am talking about the prepareAndSave

Answer 1

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.

• Jack: 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: 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?
• janos: 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.

Answer 2

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

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

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

• Jack: 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).

Answer 3

Both examples, A & B are not very nice.

There is a lot of business logic in the controller. Code in controllers is not re-usable.

You can put the validation code into the model (as a method) to make it more re-usable.

In general: Controllers should be small and contain little/less code Models should contain all business logic including validation rules