3
\$\begingroup\$

I am working on a system and I can see myself using repetitive code that does not look right. I want to clean up my code but I don't really know many ways to do so. I have 2 methods (store and update). They both look like this

store

public function store(Request $request)
    {
        $user = Auth::user();

        $validatedData = $request->validate([
            'street' => ['required', 'string'],
            'number' => ['required', 'string'],
            'city' => ['required', 'string'],
            'state' => ['required', 'string'],
            'postal_code' => ['required', 'string'],
            'country' => ['required', 'string'],
            'phone' => ['required', 'string']
        ]);

        $addresses = Address::all();

        $billing = 0;
        if($request->is_billing) {
            $billing = $request->is_billing;
            foreach($addresses as $address) {
                if($address->is_billing == 1) {
                    $address->is_billing = 0;
                    $address->save();
                }
            }
        }

        $address = Address::create([
            'user_id' => $user->id,
            'token' => Str::random(32),
            'street_name' => $request->street,
            'house_number' => $request->number,
            'postal_code' => $request->postal_code,
            'state' => $request->state,
            'city' => $request->city,
            'country_id' => $request->country,
            'phone' => $request->phone,
            'is_billing' => $billing
        ]);

        return redirect('/dashboard/user/' . $user->user_token . '/addresses');
    }

update

public function update(Request $request, $id)
    {
        $addresses = Address::all();
        $address = Address::where('token', $id)->firstOrFail();
        $user = Auth::user();

        $billing = 0;
        if($request->is_billing) {
            $billing = $request->is_billing;
            foreach($addresses as $item) {
                if($item->is_billing == 1) {
                    $item->is_billing = 0;
                    $item->save();
                }
            }
        }

        $address->user_id = $user->id;
        $address->street_name = $request->street;
        $address->house_number = $request->number;
        $address->postal_code = $request->postal_code;
        $address->state = $request->state;
        $address->city = $request->city;
        $address->country_id = $request->country;
        $address->phone = $request->phone;
        $address->is_billing = $billing;

        $address->save();

        return redirect('/dashboard/user/' . $user->user_token . '/addresses');
    }

Currently, the code looks messy and I have a feeling it can be done much more efficiently. Can someone give me tips on how to clean this up?

\$\endgroup\$

1 Answer 1

Reset to default
2
\$\begingroup\$

No need to get all data from database. Instead, only update those rows that need to be updated. Only if needed, you should work with database, avoid otherwise. Database I/O is biggest speed consumer in web applications and generally in PHP applications [when used]. https://laravel.com/docs/master/eloquent#updates

Check this one for store method, should work way faster:

public function store(Request $request)
{
    $user = Auth::user();

    $validatedData = $request->validate([
        'street' => ['required', 'string'],
        'number' => ['required', 'string'],
        'city' => ['required', 'string'],
        'state' => ['required', 'string'],
        'postal_code' => ['required', 'string'],
        'country' => ['required', 'string'],
        'phone' => ['required', 'string']
    ]);

    $billing = $request->is_billing ?? 0;
    if ($billing) {
        Address::where(['is_billing' => 1])->update(['is_billing' => 0]);
    }

    $address = Address::create([
        'user_id' => $user->id,
        'token' => Str::random(32),
        'street_name' => $request->street,
        'house_number' => $request->number,
        'postal_code' => $request->postal_code,
        'state' => $request->state,
        'city' => $request->city,
        'country_id' => $request->country,
        'phone' => $request->phone,
        'is_billing' => $billing
    ]);

    return redirect('/dashboard/user/' . $user->user_token . '/addresses');
}

Next, insted line $user = Auth::user(), you can work with policies ( https://laravel.com/docs/master/authorization ). In docs you can see PostPolicy how's been created and yours should be named AddressPolicy. And also, you should move validation to request file created with let's say

php artisan make:request AddressStoreRequest

Again, in docs you will find how to set code there ( https://laravel.com/docs/master/validation#creating-form-requests ). That is what you can do to release controller method of code and set those code blocks in their respective classes. Although your code (way I wrote it above avoiding unnecessary DB calls) will work the same even if you don't make separate classes for form validation or for authorization.

Use this code and make similar for update method.

\$\endgroup\$

You must log in to answer this question.