Laravel CRUD Controller - code request

Question

Here is my starting point for a laravel CRUD controller.

I would appreciate it if someone could do a review and show me how an experienced developer would approach this.

I do not want to progress with my app if I am not using the best practices.

I am using Laravel 8

<?php

namespace App\Http\Controllers;

use App\Models\Employee;
use Illuminate\Http\Request;
use Illuminate\Support\Facades\Auth;
use Illuminate\Support\Facades\Input;
use Illuminate\Support\Facades\Validator;
use Illuminate\Support\Facades\Redirect;
use Symfony\Component\HttpFoundation\Session\Session;

class EmployeeController extends Controller
{
    /**
     * Display a listing of the resource.
     *
     * @return \Illuminate\Http\Response
     */
    public function index()
    {
        // get all the employees
        $employees = Employee::all();

        // load the view and pass the employees
        return view('employees.index')
            ->with('employees', $employees);
    }

    /**
     * Show the form for creating a new resource.
     *
     * @return \Illuminate\Http\Response
     */
    public function create()
    {
        return view('employees.create');
    }

    /**
     * Store a newly created resource in storage.
     *
     * @param  \Illuminate\Http\Request  $request
     * @return \Illuminate\Http\Response
     */

    public function store(Request $request)
    {
        $data = $this->validate($request, [
            'firstname' => ['required', 'string', 'max:255'],
            'lastname' => ['required', 'string', 'max:255'],
            'email' => ['required', 'string', 'max:255'],
            'phone' => ['required', 'string', 'max:255'],
            'user_id' => ['required', 'int'],
        ]);

        Employee::create($data(
            'user_id' => Auth::user()->id;
        ));

        return redirect('/employees' . $request->id)->with('success', 'Employee successfully created');
    }

    /**
     * Display the specified resource.
     *
     * @param  \App\Models\Employee  $employee
     * @return \Illuminate\Http\Response
     */
    public function show($id)
    {
        $employee = Employee::find($id);

        return view('employees.show')
            ->with('employee', $employee);
    }

    /**
     * Show the form for editing the specified resource.
     *
     * @param  \App\Models\Employee  $employee
     * @return \Illuminate\Http\Response
     */
    public function edit($id)
    {
        $employee = Employee::find($id);

        return view('employees.edit')
            ->with('employee', $employee);
    }

    /**
     * Update the specified resource in storage.
     *
     * @param  \Illuminate\Http\Request  $request
     * @param  \App\Models\Employee  $employee
     * @return \Illuminate\Http\Response
     */
    public function update(Request $request, Employee $employee)
    {
        // validate
        $rules = array(
            'title' => '',
            'firstname' => 'required',
            'lastname' => 'required',
            'email' => 'required|email',
            'phone' => 'required',
            'position' => '',
            'dob' => '',
            'gender' => '',
            'line_1' => '',
            'line_2' => '',
            'line_3' => '',
            'town' => '',
            'city' => '',
            'county' => '',
            'postcode' => '',
            'start_date' => '',
            'end_date' => '',
            'salary' => '',
            'employment' => '',
            'department_id' => '',
        );
        $validator = Validator::make(request()->all(), $rules);

        if ($validator->fails()) {

            return Redirect::to('employees/' . $employee->id . '/edit')
                ->withErrors($validator)
                ->withInput();
        } else {

            $employee->update($validator->validated());

            return redirect()->refresh()
                ->with('success', $employee->firstname . ' has been successfully updated');
        }
    }

    /**
     * Remove the specified resource from storage.
     *
     * @param  \App\Models\Employee  $employee
     * @return \Illuminate\Http\Response
     */
    public function destroy(Employee $employee)
    {
        //
    }
}

Answer 1

Displaying all records?

The index() method returns all records:

$employees = Employee::all();

This could be disadvantageous for multiple reasons - especially when there are thousands or more records:

• the query to the database could get slow, especially if there are numerous columns, joins (e.g. from relationships), etc.
• The API endpoint could become slow with all of the data being processed and output
• The UI could become slower and more difficult to use

Considering limiting the number of records displayed. Laravel makes this simple with its pagination support. A keyword search could also help limit results.

Store and Update Requests

Consider defining a form request subclass that could be used for both the store() and update() methods. Doing this can reduce redundancies in those methods and make them leaner.

The fields that need to be validated for the update could be conditionally added to the array returned by the rules() method - for example:

use Illuminate\Foundation\Http\FormRequest;

class StoreEmployeeRequest extends FormRequest
{
    public function rules()
    {
        $rules = [
            'firstname' => ['required', 'string', 'max:255'],
            'lastname' => ['required', 'string', 'max:255'],
            'email' => ['required', 'string', 'max:255'],
            'phone' => ['required', 'string', 'max:255'],
            'user_id' => ['required', 'int'],
        ];
        if ($this->isMethod('PUT')) {
            $rules += [
                'title' => '',
                'position' => '',
                'dob' => '',
                'gender' => '',
                'line_1' => '',
                'line_2' => '',
                'line_3' => '',
                'town' => '',
                'city' => '',
                'county' => '',
                'postcode' => '',
                'start_date' => '',
                'end_date' => '',
                'salary' => '',
                'employment' => '',
                'department_id' => '',
            ];
        }
        return $rules;
    }
}  

The update() method also contains an else that comes after a conditional block with a return statement:

if ($validator->fails()) {

        return Redirect::to('employees/' . $employee->id . '/edit')
            ->withErrors($validator)
            ->withInput();
    } else {

        $employee->update($validator->validated());

        return redirect()->refresh()
            ->with('success', $employee->firstname . ' has been successfully updated');
    }

The else is not needed. This allows decreasing the indentation level of the code within the block

if ($validator->fails()) {
    return Redirect::to('employees/' . $employee->id . '/edit')
        ->withErrors($validator)
        ->withInput();
}

$employee->update($validator->validated());

return redirect()->refresh()
    ->with('success', $employee->firstname . ' has been successfully updated');
    

Editing a record

The edit method looks for a record:

public function edit($id)
{
    $employee = Employee::find($id);

Perhaps the following scenario below won’t be very likely or even possible if deleting is not allowed but it is something to consider.

It would be wise to guard against the scenario where one user loads a page showing a list of records, meanwhile another user deletes a record and then the first user sends a request to edit that same record. In this case the edit method could return a not found response. With Laravel this can easily be done by using the findOrFail() method instead of find().