1
\$\begingroup\$

My question/refactor is similar to what is found here: Laravel controller for form validation skinny

However, I feel that the answer is for 5.0 and not 5.2. This question does not pertain only to one controller but considering that I am making a RESTful API I believe 1 example is sufficient so I can propagate this:

class UpdateAppointmentRequest extends Request
{
    /**
     * Determine if the user is authorized to make this request.
     *
     * @return bool
     */
    public function authorize()
    {
        return false;
    }

    /**
     * Get the validation rules that apply to the request.
     *
     * @return array
     */
    public function rules()
    {
        return [
            'id' => 'required|integer',
            'user_id' => 'required|integer',
            'status' => 'integer',
            'note' => 'alpha'
        ];
    }
}

public function update(UpdateAppointmentRequest $request)
{
    $appointment = $this->appointment->find($request->get('id'));
    if ($appointment === null) {
        return $this->notFound('Appointment Not Found');
    }
    if($request->has('user_id') === true) {
        $appointment->user_id = $request->get('user_id');
    }
    if($request->has('status') === true) {
        $appointment->status = $request->get('status');
    }        
    if($request->has('note') === true) {
        $appointment->note = $request->get('note');
    }  
    try {
        return response()->json(['created' => $appointment->save()]);
    } catch(\PDOException $e) {
        return $this->internalIssues($e->getMessage());
    }
}

My issue is the if statements. Now considering that only 2 items from the validation are required and the rest are not - I have to validate if the present of the variable is there in order to update. It works well, but it will eventually make controllers very bloated (depending on which restful resources might have more parameters).

Does anyone know of a better way to refactor this within Laravel 5.2+?

\$\endgroup\$
4
  • \$\begingroup\$ could always use ternaries just to make it look cleaner \$\endgroup\$
    – jeremy
    Aug 1, 2016 at 0:49
  • \$\begingroup\$ @Jeremy dont like using ternaries - it just makes it look cleaner but doesnt make it better. \$\endgroup\$
    – azngunit81
    Aug 1, 2016 at 4:11
  • \$\begingroup\$ is $this->appointmentan Eloquent Model? \$\endgroup\$ Aug 1, 2016 at 15:52
  • \$\begingroup\$ @MarcoAurélioDeleu yesm injected from constructor \$\endgroup\$
    – azngunit81
    Aug 1, 2016 at 16:28

1 Answer 1

2
\$\begingroup\$
public function update(UpdateAppointmentRequest $request)
{
    try {
        $data = array_filter($request->only(['user_id','status','note']));
        $appointment = $this->appointment->findOrFail($request->input('id'));
        $result = $appointment->update($data);
        return response()->json(['created' => $result]);
    } catch(\PDOException $e) {
        return $this->internalIssues($e->getMessage());
    }
}

What is wrong with this simple solution? If request does not have a field, say user_id, the only would not return that. And if there is an error while updating, the catch would catch that and return error. Why all those manual checking?

\$\endgroup\$
4
  • 1
    \$\begingroup\$ Is this a separate question, or is it a review of the OP's code? It's a little hard to tell. \$\endgroup\$
    – Jamal
    Aug 2, 2016 at 10:11
  • \$\begingroup\$ I just just thought that OP did not use findOrFail and update purposefully, so that made my tone. Sorry if it is a bit question-y, but let OP give his/her opinion. \$\endgroup\$
    – vfsoraki
    Aug 2, 2016 at 10:39
  • \$\begingroup\$ your findOrFail approach is good. that escaped me, it will take care of the check in one line. However your ONLY solution is wrong. because only takes a subset of the request. IF status is absent then you will be updating it with null values and overriding previous values. \$\endgroup\$
    – azngunit81
    Aug 2, 2016 at 13:59
  • \$\begingroup\$ @azngunit81 Look at it now. You were right, I thought it would filter it by itself, but it did not. using array_filter would solve that issue. \$\endgroup\$
    – vfsoraki
    Aug 2, 2016 at 18:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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