Best way to separate validation data

Question

I don't really have a problem with me code, more like a problem with how my code is structured.

My project is written in Laravel and I use the resource controllers that laravel generate for us. Inside my Update() function I do a lot of data processing.

Some examples:

Update()

public function update(Request $request)
{

    $post = (object) $request->all();

    unset($post->XDEBUG_SESSION_START);

    $this->preparePassword($post);

    $this->prepareMail($post);

    $this->prepareAvailability($post);

    $this->prepareZip($post);

    $this->prepareSofttags($post);

    $user = User::find(Auth::id());

    $user->update((array) $post);
}

The update calls functions to process the data. Some examples:

private function prepareAvailability(stdClass &$data) : void
{
    // If no days or a date is selected, return
    if (empty($data->available_days) && empty($data->available_from)) {
        return;
    }

    // when days are given (available)
    if ($data->available_days) {
        $days = (int) $data->available_days;
        $avail_level = 0.2 * $days;

        $data->avail_level = $avail_level;
        $data->avail_from = mysqldate(strtotime('today'), true);

        unset($data->available_days);
    }

    // When a date is given (no available)
    if ($data->available_from) {
        $data->avail_level = 0;
        $data->avail_from = mysqldate(strtotime($data->available_from), true);

        unset($data->available_from);
    }
}

and:

private function prepareZip(stdClass &$data) : void
{
    if (empty($data->zip)) {
        return;
    }

    $zip4d = (int) substr($data->zip, 0, 4);
    $zipData = Zip::check($zip4d);
    $data->city = $zipData->name;
}

I just showed you 2 functies even tho I have a lot more and there will be more. Now my controller is filled with private functions that just doing some processin data in many ways.

Is there a better way, I was thinking about traits but traits should be compatible with every class. Not specific to one controller.

Any thoughts about this anyone?

P.S. I use some Helper functions if you dont understand some function that are used in the code like 'mysqldate'

Answer 1

From what I can see your controller has a lot of responsibilities, which usually means it's a good time to split things up. I think the following can be extracted to other classes:

1. Input validation (checking if the data is there). For instance available_days is a required field. For this laravel form request validation can be used.
2. Data validation (checking if the data is correct). For instance the zipcode should be split up in numbers and letters. For this you could introduce value-objects for zipcode like this example
3. Domain validation (contains the rules for your problem domain) For instance if no available_from date is given, available_from will be today). These are usually classes which make sure they can not be in an invalid state (meaning after constructing these are always complete and the data make sense). They do so by enforcing domain rules in the constructor and in other methods which modify the data they contain.

As a bonus, when splitting responsibility up like this it'll usually mean (unit)testing will get easier.

If you'd like more on this, let me know. For now I just did a quick write up.

Answer 2

With the advice I got a started working with the Laravel FormRequest class/method/concept

https://laravel.com/docs/5.8/validation#form-request-validation

I cleaned my code up and my update function is now a 'one liner', as you could see it was a mess before. Result:

public function update(UpdateUserRequest $request)
{
    user()->update($request->all());
    // user() helper function short for Auth::user()
}

Little explanation. My $request is not a Request class but an UpdateUserRequest class:

update(UpdateUserRequest $request)

'php artisan make:request UpdateUserRequest'

This class has a function called

public function withValidator($validator)

This functions is triggered when the request is made, so before it enters the controller logic, in this class I validate the data and reassign the $request values like I did before.

End result:

public function withValidator()
{
    $post = (object) $this->all();
    // Update password if possible
    $this->preparePassword($post);
    // Update mail
    $this->prepareMail($post);
    // Update availability
    $this->prepareAvailability($post);
    //
    $this->prepareZip($post);
    //
    $this->prepareSofttags($post);

    $this->merge((array) $post);
}

All the functions that all called are private function inside this class. So I moved the private function from the controller to here so my controller is clean again.

I am gonna change a bit more in this code base, cause right now I insert all the request data in every private function, but its better to just insert the data that I want to check and change the $request ($this in this case) direct after the validation. So I don't have the merge it in the end.

P.S. My english is not great, so if I have some typos or don't really understand what am saying, please ask me about it.

P.S.S If you see anything that I can improve, don't hold back and tell me :)

Thanks everyone for the advice!