# My boss re-wrote my PHP/Yii controller code. Which is better?

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) {
$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',
);
$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;
}

/**
*
* @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 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) {
$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)) {
$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();

$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();
}
}

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

-
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
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
@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

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

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

-
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

-
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