Comparison of PHP/Yii controllers

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) {

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.

StackExchange Code Review Q#60206, answer score: 11