refactor controller code

Yii 2.0 General Discussions
1

Greeting,

This is the code for actionCreate in a controller . It is messy i think. It has many nested if, a foreach, and quite long. It actualy cleaner than other action code.

Depending on the form complexity, my controller action code can be clean or ridiculously messy. However, it is not an excuse to write shitty code.

I want to throw my code out there and want to know your opinions. What can I improve ?

Note: the code inside the if ($model->load(Yii::$app->request->post())) are used in actionUpdate as well.





    public function actionCreate()

    {        

        $model = new EstimateActivityItem();

        $model->created_by = $model->updated_by = Yii::$app->user->id;

        $model->estimated_quantity = $model->estimated_man_hours 

                = $model->crew_size = $model->estimated_man_hour_per_unit 

                = $model->estimated_overall_cost_per_unit = $model->total_cost = 0;

        

        $model->estimate_activity_id = Yii::$app->request->get('estimateActivityId');   

        $estimateActivity = EstimateActivity::findOne($model->estimate_activity_id);

        

        if ($estimateActivity == null || !isset($estimateActivity->estimate->project->id)) {

            return;

        }

        

        $model->_projectId = $estimateActivity->estimate->project->id;

        

        if ($model->load(Yii::$app->request->post())) {

            $costs = Yii::$app->request->post('costs');

            

            if (empty($costs)) {

                echo 2;

                return;

            }

                        

            $transaction = Yii::$app->db->beginTransaction();

            

            try {

                if ( $flag = $model->save() ) {

                    

                    $costCode = CostCode::find()->where(['id' => $model->cost_code_id])->one();

                    $costCodeTypeArray = $costCode->costCodeTypeArray;

                    

                    $labourCost = 0;

                    

                    foreach($costs as $costCodeTypeId => $cost) {

                        if (!in_array($costCodeTypeId, $costCodeTypeArray)) continue;

                        

                        $itemCost = new \app\models\EstimateActivityItemCost();

                        $itemCost->estimate_activity_item_id = $model->id;

                        $itemCost->cost_code_type_id = $costCodeTypeId;

                        $itemCost->value = $cost;

                        $model->total_cost += $cost;

                        

                        if ( $costCodeTypeId == \app\models\CostCodeType::TYPE_5_LABOUR_COST ) {

                            $labourCost = $cost;

                        }

                        $itemCost->created_by = $itemCost->updated_by = Yii::$app->user->id;

                        if (! ($flag = $itemCost->save()) ) {

                            $transaction->rollBack();

                            break;

                        }

                    } 

                   

                    $model->estimated_man_hour_per_unit = round ( $model->estimated_man_hours / $model->estimated_quantity, 2);

                    $model->estimated_overall_cost_per_unit = round ($model->total_cost / $model->estimated_quantity, 2);

                    

                    if ($model->estimated_man_hours == 0) $model->estimated_cost_per_man_hour = 0;

                    else $model->estimated_cost_per_man_hour = round ($labourCost / $model->estimated_man_hours, 2);

                    

                    $model->update();

                }

                

                if ($flag) {

                    $transaction->commit();

                    echo 1;

                } else {

                    echo 0;

                }

            } catch (Exception $e) {

                $transaction->rollBack();

            }

        } else {

            return $this->renderAjax('create', [

                'model' => $model,

            ]);

        }

    }
2

You should move some of the logic to the model. Look into events & behaviors.

One thing you can do is remove (if these fields are on your forms remove them there too)


$model->created_by = $model->updated_by = Yii::$app->user->id;

In your EstimateActivityItem Model add




public function behaviors()

{

    return [

        [

            'class' => \yii\behaviors\BlameableBehavior\BlameableBehavior::className(),

            'createdByAttribute' => 'created_by',

            'updatedByAttribute' => 'updated_by',

        ],

    ];

}

But you’d have to post your forms and models to to see a 360 view of what’s going on to really get accurate advice.

i.e.


$costCode = CostCode::find()->where(['id' => $model->cost_code_id])->one();

$costCodeTypeArray = $costCode->costCodeTypeArray;

you might be able to do


$costCodeTypeArray = CostCode::find()->where(['id = :id', [':id' => $model->cost_code_id]])->asArray()->one();

but I don’t know what costCodeTypeArray function is doing. At a minimun you should bind the ID if it is from user input.


$costCode = CostCode::find()->where([['id = :id', [':id' => $model->cost_code_id]])->one();

$costCodeTypeArray = $costCode->costCodeTypeArray;

With the above being said you shoud bind this query too (for security reason)


$model->estimate_activity_id = Yii::$app->request->get('estimateActivityId');   

        $estimateActivity = EstimateActivity::findOne($model->estimate_activity_id);

so to this


$model->estimate_activity_id = Yii::$app->request->get('estimateActivityId');   

        $estimateActivity = EstimateActivity::find()->where([['id = :id', [':id' => $model->estimate_activity_id]])->one();

;

You could also add some of the items you are setting default values to 0 that way if the fields are empty they will get at least a 0 for value.

In your EstimateActivityItem Model


	public function rules() {

    	return [        	[['estimated_quantity','estimated_man_hours','crew_size','estimated_man_hour_per_unit','estimated_overall_cost_per_unit','total_cost'], 'default','value'=>0]

    	];

	}

You should be able to remove




$model->estimated_quantity = $model->estimated_man_hours 

                = $model->crew_size = $model->estimated_man_hour_per_unit 

                = $model->estimated_overall_cost_per_unit = $model->total_cost = 0;

Because it’s before the post so all it’s doing is setting an initial value which the rule will take care of now unless there was a specific reason on the form it’s 0

there are a couple places where you can do the above as well.

I’d try some of those things then repost what you have with the form and model too.

3

will do. thanks for advices