1
\$\begingroup\$

I'm using Laravel and this PHP method is in a controller. It's the back-end for a edit profile page. The user can change the profile picture, reset the password, the username, and so on. I'm new in PHP and would like to know if there is a better or simpler way to code this.

public function update(Request $request){

        $user = Auth::user();

        $username = $request->username;
        // PENDING $email = $request->email; 
        $wallet_address = $request->wallet_address;
        $about = $request->about;
        $password = $request->password;
        $website = $request->website;
        $location = $request->location;

        if($username){ 
            $user->username = $username; 
            $user->save();
        }

        /** upload avatar section */
        
        $image = $request->file('avatar');

        if($image){

            // note: if avatar_name variable is null it means the user has never uploaded an image..
            // if the user has uploaded an avatar before, delete it before uploading the new one..
            if($user->avatar_name){
                Storage::delete('/public/profile-pics/'.$user->avatar_name);
            }

            $image_new_name = date('dmy_H_s_i').'_'.$user->id.'_'.$image->getClientOriginalName();
            $image->storeAs('profile-pics',$image_new_name,'public');
            $user->avatar_url = 'storage/profile-pics/'.$image_new_name;
            $user->avatar_name = $image_new_name;
            $user->save();
        }

        if($wallet_address){
             $user->wallet_address = $wallet_address;
             $user->save(); 
        }

        if($about){
             $user->about = $about; 
             $user->save();
        }

        if($password){
             $user->password = $password; 
             $user->save();
        }

        if($website){
            $user->website = $website; 
            $user->save();
        }

        if($location){
             $user->location = $location; 
             $user->save();
        }

        return redirect()->route('edit_profile')->with('success','Your account has been updated successfully.');
    }
\$\endgroup\$
4
  • \$\begingroup\$ Welcome to Code Review! The current question title, which states your concerns about the code, is too general to be useful here. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How do I ask a good question?, as well as How to get the best value out of Code Review: Asking Questions for guidance on writing good question titles. \$\endgroup\$ Apr 14 '21 at 14:36
  • \$\begingroup\$ Thank you Sam. I don't get it.. isn't this site purpose supposed to be for reviewing code as the url states? .. The title of the questions describes exactly what I wish.. For someone to review the code and make some recommendations or suggestions for improvement.. \$\endgroup\$
    – Gass
    Apr 14 '21 at 19:44
  • \$\begingroup\$ Yes the purpose of the site is reviewing code. However, as I mentioned before, the standard is for the title to describe what the code does. The majority of posts on this site are about improving code so if we all had titles about code review then it would be difficult to distinguish between them. I changed the title accordingly. Feel free to edit and give it a different title if there is something more appropriate to describe what the code does. \$\endgroup\$ Apr 14 '21 at 20:57
  • \$\begingroup\$ That makes sense. Thanks Sam. \$\endgroup\$
    – Gass
    Apr 15 '21 at 10:42
2
\$\begingroup\$

Redundant (and likely inefficient) code

There are multiple blocks that repeat:

    if($username){ 
        $user->username = $username; 
        $user->save();
    }

    if($wallet_address){
         $user->wallet_address = $wallet_address;
         $user->save(); 
    }

    if($about){
         $user->about = $about; 
         $user->save();
    }

    if($password){
         $user->password = $password; 
         $user->save();
    }

    if($website){
        $user->website = $website; 
        $user->save();
    }

    if($location){
         $user->location = $location; 
         $user->save();
    }

The only thing that really changes here is the property/field being checked. That could be simplified using an array containing the field names:

//could be moved to the model or another namespace
const FIELDS = ['username', 'wallet_address', 'about', 'password', 'website', 'location'];
public function update(Request $request){
    $user = Auth::user();
    foreach (self::FIELDS as $field) {
        if ($request->$field) {
            $user->$field = $request->$field;
            $user->save();
        }
    }
    //handle avatar image update
}

Obviously that shows that the save() method is called multiple times, which could be inefficient if it has to write data (e.g. to a database or other data store). You could conditionally call the save method if one of those fields has a value and is updated on the user. The save method is also called when an image is included in the request (i.e. when $request->file('avatar') has a non-null value). That boolean could be combined with that condition as well, which would lead to a single point of updating the user model.

Validation of the Request

You could create a Request subclass to validate the fields that should be (sometimes) present on the request

For example, you could run

php artisan make:request StoreUserRequest

And that would create StoreUserRequest in the app/Http/Requests directory which would extend Illuminate\Foundation\Http\FormRequest. The boilerplate would give it an empty rules() method that could contain the fields that can be passed. For Example:

public function rules()
{
    return [
        'username' => 'sometimes',
        'wallet_address' => 'sometimes',
        'about' => 'sometimes',
        'password' => 'sometimes',
        'location' => 'sometimes',
    ];
}

And those can container other rules - for example, if a field is required, has a maximum string length, etc.

Then in the controller method use that new request subclass as the type for $request.

\$\endgroup\$
5
  • \$\begingroup\$ Thank you Sam, this is very useful. I'm getting an error with const FIELDS and foreach error syntax error, unexpected 'const' (T_CONST) any suggestions? \$\endgroup\$
    – Gass
    Apr 15 '21 at 11:06
  • \$\begingroup\$ Ah yes- the constant needs to be defined outside the method. I have updated that snippet \$\endgroup\$ Apr 15 '21 at 12:41
  • 1
    \$\begingroup\$ Okay now it's working. Just one suggestion for edit. There is a } missing on the end of that snippet. Thanks Sam. \$\endgroup\$
    – Gass
    Apr 15 '21 at 13:06
  • \$\begingroup\$ @Samonela could you explain me why is it necessary to use self:: I've been struggling to understand it. \$\endgroup\$
    – Gass
    Aug 22 '21 at 8:26
  • 1
    \$\begingroup\$ @Gass that is used to specify where the constant is define- self:: is used because it is in the same class. In your code it also references static methods in the same manner: Auth::user(), Storage::delete(), etc. Read more about scope resolution operator, Late Static Bindings as well as When to use self over $this? \$\endgroup\$ Aug 28 '21 at 21:17
4
\$\begingroup\$

So the above answer from Sᴀᴍ Onᴇᴌᴀ already covers quite a few points. I've worked with Laravel for the past 4 years, so I'll go a bit further in depth on how I'd approach this, including a few suggestions.

First you'll want to use RequestValidation as Sᴀᴍ Onᴇᴌᴀ indicated, this will help guard against invalid input. And make use of Laravel's own input validation logic (sends the user back to the form with error messages in your message bag)

Secondly you have a lot of logic on the User model related to an avatar. I'd suggest creating an Image / Avatar Model that handles the image part of it all, in the end you'd end up with an image_id/avatar_id on the User model as a relation. That way you can have all the logic that handles a file on your filesystem and the associated data. It also enables you (if you go for image) to reuse the logic in a different context. And declutters your user model

Thirdly, as Sᴀᴍ Onᴇᴌᴀ indicated you do a lot of ->save() calls. Each call is a database round trip. And a costly action. You can set each property seperately if you want and at the end of your logic you can call ->isDirty() on your user model, if it returns true, data has changed from the database returned data, indicating an update should be done. At this point you could save.

I however would suggest building an update array of the data to update, and calling the ->update() method with an array containing the key => value pairs that you wish to update. You can check this data array for emptyness to avoid unneccessary updates.

I would always suggest having some form of validation when changing passwords, to make sure an unauthorized user can't simply change the password. (a simple enter current password would do here)

I'll refactor the code you had and try to leave it inside the existing method.

public function update(Request $request)
{
    $user = Auth::user();
    // Read up on the Laravel Request Validation for available rules and usages
    $request->validate([
        'username'         => 'sometimes',
        'email'            => 'email|sometimes',
        'wallet_address'   => 'sometimes',
        'about'            => 'sometimes',
        'password'         => 'sometimes',
        'current_password' => 'required_with:password',
        'website'          => 'sometimes',
        'location'         => 'sometimes',
    ]);
    // $request->input() will return null if the given key is not found
    // I use an array_filter here to filter out empty results, if you expect falsey values (like null to unset something, or 0/false/'') you should use a different approach
    $update_data = array_filter([
        'username'       => $request->input('username'),
        'email'          => $request->input('email'),
        'wallet_address' => $request->input('wallet_address'),
        'about'          => $request->input('about'),
        'password'       => $request->input('password'),
        'website'        => $request->input('website'),
        'location'       => $request->input('location'),
    ]);
    if ($request->input('password')) {
        // enter logic here to validate existing password
        // if this logic fails, you should just return instead of saving anything
    }
    if (!empty($update_data)) {
        // You can store the result to see if you successfully updated
        $update_status = $user->update($update_data);
    }
    // Check if an file has been given
    if ($request->hasFile('avatar')) {
        // I'd suggest moving the image handling to a seperate method at least.
        $this->handleImage($user, $request->file('avatar'));
    }
    if (!$update_status) {
        return redirect()->route('edit_profile')->with('failure','Your account could not be updated.')
    }
    return redirect()->route('edit_profile')->with('success','Your account has been updated successfully.')
}

private function handleImage(User $user, UploadedFile $image)
{
    // if the image is a seperate relation, you could delete the existing relation
    // on the delete for the relation you could hook in a cleanup for the file.
    if ($user->avatar_name) {
        // You can add a "disk" to your filesystem configuration where you can set the default path.
        Storage::delete('/public/profile-pics/' . $user->avatar_name);
    }
    $image_new_name = date('dmy_H_s_i') . '_' . $user->id . '_' . $image->getClientOriginalName();
    $image->storeAs('profile-pics', $image_new_name, 'public');
    // if you use a properly configured Storage::disk you can only store the name, and the disk will resolve where to look for the file
    // Storing paths in the database binds your database to your filesystem structure making future changes more difficult to handle (think moving images to a seperate location, or a different server)
    // Another point, if you expect many users, consider using a subdivision in your path (something like the first few digits of the user id or anything that'll let you break up the files into subdirectories), most systems do weird things once you deal with 200k+ files in a single directory.
    $user->avatar_url = 'storage/profile-pics/' . $image_new_name;
    $user->avatar_name = $image_new_name;
    $user->save();
}
\$\endgroup\$
1
  • \$\begingroup\$ Thank you @Tropus this is very useful. I'll have to give it sometime to digest all your suggestions. I'm knew to Laravel so I want to take my time to get familiarized with it. \$\endgroup\$
    – Gass
    Apr 15 '21 at 11:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.