Constructive feedback request -- Controller Method Optimization

1. I see no problem in the way you are validating. You could do something like the following but that should not change much.

$validatedAttributes = $request->validate([
    'first_name' => 'required|max:255',
    'last_name' => 'required|max:255',
    'employee_classification' => 'required|max:50',
    'hourly_or_salary' => 'required',
    'wage' => 'required|numeric'
]);

2 . I'm ok with having the controller with the responsability of saving the Employee to the database.

3 . I would extract that to the model, like so:

    // Model
    public function calculateEarnings($wage)
    {
        if ($this->is_salary === 0) {
            $this->hourly_rate = $wage;
            $this->salary = $wage * 40 * 52; // 40 hrs/week, 52 weeks/yr
        } else {
            $this->hourly_rate = $wage / (40 * 52);
            $this->salary = $wage;
        }
    }


    // Controller
    $employee->calculateEarnings($request->wage);

4 . If those calculations are heavy in terms of computation effort AND are not supposed to change too often, I would move them to the database. Otherwise, I would calculate in real time.

Regarding the other topics, I see no problem.

I would just extract this logic into the model constructor:

    public function __construct()
    {
        $this->guaranteed_40 = 0;
        $this->crew_id = 0;
        $this->is_deleted = 0;
    }

Thank you very much for the response! In response to the model constructor (your final comment), I have been thinking of using this code at the top of my model:

protected $attributes = [ 'guaranteed_40' => 0, 'crew_id' => 0, 'is_deleted' => 0 ];

I think it will accomplish the same thing you're mentioning with the constructor?

Yes, you're right. I have not tested it myself, but if you look at the constructor of the \Illuminate\Database\Eloquent\Model class, you'll find this

(...)
$this->fill($attributes);

So as long as you are not overriding the constructor, or calling parent::__construct() if you are overriding it, that should work too.

Great explanation! Makes me feel more confident hehe

As you've type hionted the request class anyway, you may aswell create a form request class. I personally think that validation done in controllers results in unecessary duplication.

For the toaster messages, that you mention you use in every controller you could create a RedirectResponse macro. This would allow you to create a toaster notifiucation and redirect using the following syntax:

return redirect()->route('employees.index')->created();

The macro itself need to go into a service provider (assuming the toaster class just sets flash data) i.e.

` RedirectResponse::macro('created', function ($route) { Toastr::success('Employee has been added.', 'Success!'); return $this->with('success', 'The record was created'); });

`

For the calculations, again they're fine in the controller ina small project. I personally move satuff like that to Eloquent observers, meaning you could remove lines 21 - 27 from the controller completely.

I'll look into Eloquent Observers. I'm not familiar with them currently. I'll also look into RedirectResponse macros. Thanks!

@jcgivens21 Looks like a good start. In response to your actual questions:

$this->validate: Should I move this validation logic into a Request class? Or is this generally overkill?

I always place validation in form request classes. I prefer my controllers to only be dealing with a valid request, not validate the request and then do the processing.

I'm creating the Employee here and saving it directly in the controller. Is it better to decouple this logic? I feel I've "slimmed" my controller based on the number of methods, but there's still a huge amount of text here, and I'm not sure if this is the best place to handle this Employee saving logic.

Instead of assigning each property individually, you could make your attributes fillable in your model and then use create():

$employee = Employee::create($request->validated());

Note: ->validated() will only work if your request does the validating.

In line with #2, there is some condition-based logic that when the user is inputting the employee data, they select whether the employee is paid Hourly or by Salary, and then I accept either of those values from the form, and I also calculate the other value even if it isn't sent and store both in the database table, in addition to whether it "is_salary" so I know when displaying the employee whether or not to display the values as hourly or salary. Condition-based logic like this is best in this controller logic?

This depends on a case-by-case basis. If I find there’s lots of conditionally branching, I may move the code that deals with persisting a record to its own job class, and then dispatch that synchronously:

$employee = CreateEmployee::dispatchNow($request->validated());

Also with #3, there is some calculations being done in addition to the condition-based logic. Should calculation results be stored in the model/database tables or should those be handled real-time? It was simpler when I started to just store everything in the table without worrying about calculating these values upon request--but in proper design, should only 1 of those values be saved to the database and the other always calculated?

Again, it depends on a case-by-case basis. In this case, you seem to be manipulating one of two values depending if the employee is salaried or not. You could maybe move this logic to an mutator in that case:

public function setWageAttribute($value)
{
    if ($this->is_salary == 0) {
        $this->hourly_rate = $value;
        $this->salary = $value * 40 * 52;
    } else {
        $this->hourly_rate = $value / (40 * 52);
        $this->salary = $value;
    }
}

This of course depends on is_salary being set on your Employee model instance first.

$employee->is_salary = 1;
$employee->wage = 40000;

Once the employee is added, I have a Toastr notification (basically a popup) that informs the user that their form submission was successful. I do this in basically EVERY function in the program, and in my Layouts.app blade template, it just picks up these Toastr notifications. I feel this is a very good implementation, and it works well for me, but I'm open to criticisms of this.

My only criticism is, it’s directly tied to an implementation. Personally, I just redirect and set success or error messages in the session:

return redirect()->route('event.index')->withSuccess('Event was deleted.');

I’ll then have the view do something with that, i.e. display it in the header, as a toast pop-up, etc. If I want to change how flash messages are displayed on the front-end, then I just have to do it there rather than go through all of my controller actions and change instances of Toastr::success() to whatever I’m going to use instead.

On the employee creation submit form, there are 2 submit buttons. One button (which is labeled "Submit" is just to submit the employee and then redirect the user back to the page that displays all employees. The second button is labeled "Submit & Return To This Page" and its function is to return the user back to the same form so that they don't have to click an additional button to get back here to add more employees. In the controller, I just check which button was submitted and redirect based on which button was pressed. Any thoughts on this setup?

No real thoughts. It's conditionally logic, so it's fine to have an if statement change where the controller redirects the user to.

Other general thoughts?

None other than those above! Essentially:

• Put validation in form requests.
• Try to use mass-assignment rather than setting properties individually.
• Try to keep things generic, rather than tied to specific implementations.

Following the above, you may end up with controller actions that look like this:

public function store(StoreEmployeeRequest $request)
{
    $employee = Employee::create($request->validated());

    $url = $request->add_and_return_button ? back() : route('employee.show', $employee);

    return redirect()->to($url)->withSuccess('Employee has been added.');
}

@martinbean: Holy information batman! That is EXACTLY the kind of feedback I envisioned for this post. Thank you so much. It'll take a while to digest and implement, but your representation of the end result really exemplifies what I envision that method should look like.

@jcgivens21 No problem. Glad you found it useful :)