danny620's avatar

better refactor this code

How can i better refactor this code

public function updateProfileImage(ProfileImageFormRequest $request)
{

    $uploadPath = base_path() . '/public/uploads/profile/'; // upload path

    $extension = $request->file('photo')->getClientOriginalExtension();
    $fileName = time() . '.' . $extension;
    $request->file('photo')->move($uploadPath, $fileName);

    $image = \Image::make($uploadPath . $fileName)->resize(260, 260)->save();

    $user = User::findOrFail(Auth::user()->id);
    $user->fill(['profile_img' => $fileName])->save();
    return redirect('settings/details')->with('msg', 'Successfully updated profile image!');
}
0 likes
14 replies
anon34372's avatar

@northplanet I think the code is perfectly okay!

Only thing you can do here is to create...well, just a method in User class that would handle the whole process. So you would end up with something like this:

public function updateProfileImage(ProfileImageFormRequest $request)
{
    Auth::user()->setProfileImage($request->file('photo')); 
   
    return redirect('settings/details')->with('msg', 'Successfully updated profile image!');
}

I believe user has to be logged in to change his avatar so you probably don't have to do anything like this: $user = User::findOrFail(Auth::user()->id);

danny620's avatar

@JesseVeritas what do you mean by "I believe user has to be logged in to change his avatar so you probably don't have to do anything like this: $user = User::findOrFail(Auth::user()->id);"

zachleigh's avatar

You dont need this:

$user = User::findOrFail(Auth::user()->id);

Auth::user() gives you an instance of the current user so there's no point in looking up the user.

$user = Auth::user();
2 likes
danny620's avatar

I've tried but no luck

public function setProfileImage($photo)
{
    $uploadPath = base_path() . '/public/uploads/profile/'; // upload path

    $extension = $photo->file('photo')->getClientOriginalExtension();
    $fileName = time() . '.' . $extension;
    $photo->file('photo')->move($uploadPath, $fileName);

    $image = \Image::make($uploadPath . $fileName)->resize(260, 260)->save();

    $this->profile_img = $fileName;
    $this->save();
}
anon34372's avatar

@northplanet

Any errors? And I think you need to fix this:

//$extension = $photo->file('photo')->getClientOriginalExtension();
$extension = $photo->getClientOriginalExtension();

//$photo->file('photo')->move($uploadPath, $fileName);
$photo->move($uploadPath, $fileName);

Or instead you can change this:

Auth::user()->setProfileImage($request->file('photo')); 
//to
Auth::user()->setProfileImage($request); 

I would go with the first option :-)

spekkionu's avatar

You don't need to move the file before creating the image instance out of it.
You can create it directly from the file object.

$image = Image::make($request->file('photo'));

You then just need to provide the path and filename for the save method

Also note with your current code if two people upload images within a second of each other the filenames could be the same and the second one would overwrite the first one.

To prevent this you could do something like prepend the filename with the user id or use a uuid.

$fileName = $user->id . '-' . time() . '.' . $extension;
Javi's avatar

You can also create a new class ProfileImage which would extend from Image and do all the stuff in its constructor or a factory method.

UhOh's avatar

is all of that in a controller method? (seems so, form request...)

you'd want to shift it out to a repo|service|class etc

danny620's avatar

I need to use the same code in another controller what would be the best approach so i'm not repeating myself?

spekkionu's avatar

Here is how I tend to handle something like this so that it is reusable and easy to modify.

First rather than saving the file directly to the filesystem I make use of the Laravel Filesystem component.
I would add a filesystem disk for your uploads directory. If you open config/filesystems.php you can add the following to the disks array.

'uploads' => [
    'driver' => 'local',
    'root' =>  base_path() . '/public/uploads',
    'visibility' => 'public'
],

This gives you the ability to move the files to cloud storage or use a cdn by ony changing some configuration and not having to touch your code.

The next thing I do is create a class that will handing the resizing and saving of the image file itself.
The class should receive Intervention\Image\ImageManager as a parameter in its constructor.
Then create a method in this class that receives Illuminate\Contracts\Filesystem\Filesystem, Symfony\Component\HttpFoundation\File\UploadedFile, and App\User (or whatever model the file is attached to) as parameters.
This method should determine the filename of the image, load the image from the uploaded file and resize it, then save it using the filesystem object.

namespace wherever\you\put\it;

use Intervention\Image\ImageManager;
use Illuminate\Contracts\Filesystem\Filesystem;
use Symfony\Component\HttpFoundation\File\UploadedFile;
use App\User;

class ProfileImageSaver
{
    private $image;

    public function __construct(ImageManager $image)
    {
        $this->image = $image;
    }

    public function saveProfileImage(UploadedFile $file, Filesystem $filesystem, User $user)
    {
        $extension = $file->getClientOriginalExtension();
        $filename = $user->id . '-' . time() . '.' . $extension;
        $image = $this->image->make($file)->resize(260, 260)->encode($extension);
        $path   = "profile/{$filename}";
        $filesystem->put($path, $image->getEncoded());
        $image->destroy();
        return $filename;
    }
}

Your controller action would look something like this.

public function updateProfileImage(ProfileImageFormRequest $request, ProfileImageSaver $imageSaver)
{
    $user = Auth::user();
    $filename = $imageSaver->saveProfileImage($request->file('photo'), Storage::disk('uploads'), $user);
    $user->profile_img = $filename;
    $user->save();

    return redirect('settings/details')->with('msg', 'Successfully updated profile image!');
}
1 like
tylernathanreed's avatar

Here's what I would do:

First off, I'd create a Profile Controller, allowing you to split up your logic:

ProfileController.php

namespace App\Http\Controllers;

use App\Http\Requests\ProfileImageFormRequest as ProfileRequest;
use App\Http\Controllers\Controller;
use Image;
use Auth;

class ProfileController extends Controller
{
    /**
     * The Path to be appended to the Base Path.
     *
     * @var string
     */
    protected $path = '/public/uploads/profile/';

    /**
     * Creates and Persists a Profile Image using the specified Photo.
     *
     * @param  File  $photo  The specified Photo, typically generated from a Request.
     *
     * @return string  The Filename of the new Image.
     */
    public function create($photo)
    {
        // Determine the Upload Path
        $path = base_path() . $this->path;

        // Determine the Name of the File
        $name = time() . '.' . $photo->getClientOriginalExtension();

        // Persist the Image to Disk
        return $this->store($photo, $path, $name);

    }

    /**
     * Stores the specified Photo at the specified Path using the specified Name.
     *
     * @param  File    $photo  The specified Photo, typically generated from a Request.
     * @param  string  $path   The Directory Path where the Photo should be stored.
     * @param  string  $name   The Name to give the Photo when it is stored.
     *
     * @return  string  The Filename of the new Image.
     */
    public function store($photo, $path, $name)
    {
        // Persist the Image to Disk
        $photo->move($path, $name);

        // Resize the Image
        Image::make($path . $name)->resize(260, 260)->save();

        // Return the Name of the File
        return $name;
    }

    /**
     * Displays the Form for the User to Edit their Profile Picture.
     *
     * @return Response
     */
    public function edit()
    {
        return view('users.profile.edit'); // Or whatever your view is
    }

    /**
     * Updates the Authenticated User's Profile with the one specified in the Request.
     *
     * @param  ProfileRequest  $request  The Update Request.
     *
     * @return Response
     */
    public function update(ProfileRequest $request)
    {
        // Determine the Authenticated User
        $user = Auth::user();

        // Determine the Profile Image
        $image = $this->create($request->file('photo'));

        // Assign the Image to the User
        $user->profile = $image;

        // Save the User
        $user->save();

        // Redirect the User
        return redirect('settings/details')->with('msg', 'Successfully updated profile image!');
    }
}

Then I'd create magic accessors and mutators on the User to remove the use of Foreign Key column names outside of the User Model:

User.php

public function getPhotoAttribute($photo)
{
    // Return the Photo as an Image
}

public function setPhotoAttribute($photo)
{
    $this->profile_img = $photo;
}

And you could move the assignment of the User's attributes in the ProfileController from Update to Store, if you wanted. I could see it working both ways.

wiedem's avatar
    public function update(ProfileRequest $request)
    {
        // Determine the Authenticated User
        $user = Auth::user();

There's actually no need to use the Auth facade, you can simply use

    public function update(ProfileRequest $request)
    {
        // Determine the Authenticated User
        $user = $request->user();
1 like

Please or to participate in this conversation.