Controller method to store course

Question

I have this controller method to add a course and it works fine but I really don't like this code because I think it's too large and a little bit ugly

I was trying to do it cleaner and a little bit shorter, But don't have knowledge to do that so can anyone please help me with this code?

   public function storeCourse(Request $request)
    {
        $request->validate([
            'slug' => 'required',
            'name' => 'required',
            'image' => 'required',
            'description' => 'required',
            'body' => 'required',
            'audio' => 'required',
            'price' => 'required',
            'level' => 'required',
            'subtitle' => 'required',
            'knowledge' => 'required',
            'required' => 'required',
            'userId' => 'required',
            'categoryId' => 'required'
        ]);

        if ($request->hasFile('image')) {
            $filenameWithExt = $request->file('image')->getClientOriginalName();
            $filename  = pathinfo($filenameWithExt, PATHINFO_FILENAME);
            $extension = $request->file('image')->getClientOriginalExtension();
            $fileNameToStore = $filename . '_' . time() . '.' . $extension;
            $request->image->move(public_path('/uploads'), $fileNameToStore);
        } else {
            $fileNameToStore = 'no_img';
        }

        $course = new Course();
        $course->slug = $request->get('slug');
        $course->name = $request->get('name');
        $course->image = $fileNameToStore;
        $course->description = $request->get('description');
        $course->body = $request->get('body');
        $course->audio = $request->get('audio');
        $course->price = $request->get('price');
        $course->level = $request->get('level');
        $course->subtitle = $request->get('subtitle');
        $course->knowledge = $request->get('knowledge');
        $course->required = $request->get('required');
        $course->user_id = $request->get('userId');
        $course->course_category_id = $request->get('categoryId');
        $course->save();

        return redirect()->back();
    }

Answer 1

You could create 2 sub-functions for the different parts in your function.

• validate($request) so that long part is hidden
• getImageName($request)

Also you could remove all those setters for the course and put everything in the constructor:

$r = $request;
$course = new Course($r->get('slug'), $r->get('name'), ...);

Answer 2

Avoid redundant code

The fields to be set from the request could be stored in an array - perhaps a class constant:

private const COURSE_ATTRIBUTES = [
    'slug',
    'name',
    'image',
    'description',
    'body',
    'audio',
    'price',
    'level',
    'subtitle',
    'knowledge',
    'required',
    'userId',
    'categoryId'
];

Then those can be used to specify the fields the request should validate with the help of the built-in PHP function array_fill_keys():

$request->validate(array_fill_keys(self::COURSE_ATTRIBUTES, 'required'));

Then when setting the attributes of the new course the array could be used, though special handling of the last two fields would need to be added:

$course = new Course();
foreach (self::COURSE_ATTRIBUTES as $attribute) {
    $course->$attribute = $request->get($attribute);
}
$course->user_id = $request->get('userId');
$course->course_category_id = $request->get('categoryId');

Make a form request class for handling the validation

The validation rules could be moved out to a FormRequest subclass. For example:

namespace App\Http\Requests;

use Illuminate\Foundation\Http\FormRequest;

class StoreCourseRequest extends FormRequest
{
    public function rules()
    {
        return [
             'slug' => 'required',
             'name' => 'required',
             'image' => 'required',
             'description' => 'required',
             'body' => 'required',
             'audio' => 'required',
             'price' => 'required',
             'level' => 'required',
             'subtitle' => 'required',
             'knowledge' => 'required',
             'required' => 'required',
             'userId' => 'required',
             'categoryId' => 'required'
        ];
    }
}

Note that beyond required there are numerous validation rules that could be used - e.g. max, unique, etc.

If the validation fails and the request was an XHR request, an HTTP response with a 422 status code will be returned to the user including a JSON representation of the validation errors..

Then that subclass can be injected instead of Illuminate\Http\Request in the storeCourse method arguments and use $request->all() to get the fields to pass to $course->save().

If the properties that can be set on the course (e.g. slug, name, image, etc.) are declared in the fillable property on the Course model (which could be declared as a public constant that is also assigned to the protected property in the model) then you should be able to simplify the controller method like below (warning - untested):

public function storeCourse(StoreCourseRequest $request)
{
    if ($request->hasFile('image')) {
        $filenameWithExt = $request->file('image')->getClientOriginalName();
        $filename  = pathinfo($filenameWithExt, PATHINFO_FILENAME);
        $extension = $request->file('image')->getClientOriginalExtension();
        $fileNameToStore = $filename . '_' . time() . '.' . $extension;
        $request->image->move(public_path('/uploads'), $fileNameToStore);
    }
    $attributes = $request->validated();
    $attributes['user_id'] = $attributes['userId'];
    $attributes['course_category_id'] = $attributes['categoryId'];
    $attributes['image'] = $fileNameToStore ?? 'no_img';
    $course = Course::create($attributes);
    $course->save();
    return redirect()->back();
}