uniqueginun's avatar

file upload refactoring

Hey seniors:

How to refactor the following code, I mean the if-statement part

    if ($photo = $request->file('attachment')) {
       $photo->storeAs('photos', 'avatar' . time() . '.' . $photo->extension());
    }	

    if ($photo2 = $request->file('upload')) {
        $photo2->storeAs('photos', 'uploads' . time() . '.' . $photo2->extension());
    }
0 likes
9 replies
uniqueginun's avatar

this route receives requests from multiple forms. some send "attachment" some send "upload" some send them both some send more than these two. I want the uploading to be dynamic.

tisuchi's avatar

@uniqueginun First of all, if your current code works fine, I would say just follow the simple step, that currently has.

Secondly, if you submit multiple form on same action and not sure whether in future it will submit more form or not, on that situation, you can follow a separate class for each form submittion based on a contract (Interface), that will be following Open Closed Principle (Some may be refer it over engineering of it here).

That's what I can think of now.

Tray2's avatar

This will always be true

if ($photo = $request->file('attachment'))

use the hasFile method instead

if ($request->hasFile('attachment')) {
    $path = $request->attachment->storeAs('photos', 'avatar' . time() . '.' . $request->attacmhent->extension());
}
uniqueginun's avatar

what if I did:

protected $attachs = ['upload', 'attachment', 'another'];

if($request->hasFile($attachs)) {
	foreach($attachs as $attach) {
		if($request->hasFile($attach)) {
			//upload file
		}
	}
}
Tray2's avatar

Don't loop if you don't have to.

If you really want to refactor extract each file type to it's own method.

$this->saveUpload($request);
$this->saveAttachment($request);
$this->saveAnother($request);

Then take it from there further on.

uniqueginun's avatar

I didn't understand why you don't like looping, but I guess your solution is solid if we exclude looping.

I will make a dedicated function for each potential uploaded file and check inside if it's exists then upload. is that what you mean?

Tray2's avatar

yes something like that.

But I agree with @tisuchi that there is no real need for refactoring.

1 like
tisuchi's avatar

@uniqueginun The approach suggested by @tray2 looks clean enough already.

Why not you just create separate methods and handle all the logic insider them.

BTW, you can use a loop if you want, but since you can solve any issue without the loop, then what is the purpose of using it? Looping will make iteration, which may affect your performance (depends on how many times it iterate). Therefore I believe looping is unnecessary use here.

Please or to participate in this conversation.