User Avatar upload

Question

I have been looking at some coding videos on Laracasts and the focus seems to be very readable code and keeping the controller clean. How can I clean this up? This code takes an uploaded user avatar, stores the avatar image on the file system and stores the name in a database.

public function update(Request $request, $id)
{
 $this->validate($request, [
 'avatar' => 'required|image|max:10000|mimes:jpg,jpeg,gif,bmp,png'
 ]);
 $UploadedFile = $request->file('avatar');
 $name = renameFile($UploadedFile); // helper function.
 $user = User::where('id', '=', $id)->first();
 if(!is_null($user->avatar)){ // avatar's name stored in user table
 Storage::disk('public')->delete('avatars/' . $user->avatar);
 };
 $image = Image::make($UploadedFile)
 ->resize(400, null, function ($constraint) { $constraint->aspectRatio(); } )
 ->encode('jpg', 80);
 Storage::disk('public')->put('avatars/' . $name, $image);
 $user->avatar = $name;
 $user->save();
 return back()->with(message('User Profile Photo has been updated!', 'success'));
};

It works fine. It's just the presentation that's bothering me.

I have looked into repositories, interfaces etc but can't seem to figure out when to use what.

• Jeffrey: I do have other places using almost identical code. Differences are storage locations and resizing dimensions.

Answer 1

I honestly don't think there is much to "clean up" here, as this seems pretty straight forward. A few stylistic notes:

• It is pretty standard in PHP to have variables named with first letter in lower case. So consider $UploadedFile => $uploadedFile.
• Why blank line between most every line of code? This seems excessive. Vertical white space can be good to group various logical sections of code, but in your cases it seems to be overkill.
• Don't put comments at the end of lines. Put them on the line before the code to which they relate. In this case, I don;t think you even need to the comments you have.

---

From a logic standpoint, I don't think it makes sense for you to delete the existing avatar file until after you sucessfully format and write the new one. What happens if something fails during the process of transforming and writing the new image file? You have now destroyed the existing file without a valid replacement.

---

Your code is very happy-path oriented. If something fails, it is unclear how you message the user regarding that failure.

• Jeffrey: Points taken. What do you think about wrapping some of those lines into methods/functions to make them more readable?
• Mike Brant: @Pyol7 You could certainly extract logic for tings such as deleting avatar file, writing avatar file, instantiating a user by id (something that should exist in User class) into their own methods, but it is hard to say without understanding your wider application if there is re-use case for that sort of logic.
• Jeffrey: I do have other places using almost identical code. Differences are storage locations and resizing dimensions.
• Mike Brant: @Pyol7 Well then, it sounds like you have opportunity to refactor to get better re-use.
• Jeffrey: I was considering posting the relevant code but it might be too much so I will do some more reading and post my results. Thanks for the help.