Structuring code logic for events on laravel controller

Question

The code below will store a base64 image on another website and create an event to display the image on a Vue page in real time. It works fine and all.

My question is, is this a good way of doing it or there's a better way to structure the code? I've thought about placing the logic in a listener but it doesn't seem that I need to use one. For queues, I'll add them later on.

Controller

public function store(Request $request)
{
    $base64 = $request->image;
    $image = str_replace('data:image/png;base64,', '', $base64);
    $imageName = 'draw'.time().'.png';
    $response = $this->guzzleClient()->post(config('archiving.api_postBase64_file'),
        [  
            'multipart' => [
                [
                    'name' => 'file',
                    'contents' => $image
                ],
                [
                    'name' => 'file_name',
                    'contents' => $imageName
                ],
                [
                    'name' => 'file_folder',
                    'contents' => 'drawings'
                ],
            ],
        ]
    );
    if($response->getStatusCode() >= 500){
        return response($response->getBody(), $response->getStatusCode());
    }else{
        $drawing = Drawing::create([
            'user_id'=>Auth::id(),
            'file_name'=>$imageName,
        ]);
        event(new NewDrawing($drawing));
        return response('Drawing created!',200);
    }
}

NewDrawing Event

class NewDrawing implements ShouldBroadcastNow
{
    use Dispatchable, InteractsWithSockets, SerializesModels;

    public $drawing;

    public function __construct(Drawing $drawing)
    {
        $this->drawing = $drawing;
    }
    public function broadcastOn()
    {
        return new Channel('channel-drawing');
    }
 }

Frontend (Vue)

<script>
export default {
    data(){
        return{
            images:'',
        }
    },
    methods:{
        listen() {
            Echo.channel('channel-drawing')
            .listen('NewDrawing', res => {
                this.images.push(res.drawing);
            })  
        }
    },
    mounted() {
        this.listen()
    }
}
</script>

Answer 1

I'm mostly a back-end guy who fusses his way through front end code. Here's my advice:

• Keep the controller code as close as possible to handling the response only. Delegate the creation of objects to their own classes. Think of the S in SOLID - Single Responsibility.

  public function store(Request $request)
  {
      $image = new ImageMeta($base64);
      $archiver = new ImageArchiver($image);
      $response = $archiver->post();
  
      if($response->getStatusCode() >= 500){
          return response($response->getBody(), $response->getStatusCode());
      }else{
          $drawing = Drawing::create([
              'user_id'=>Auth::id(),
              'file_name'=>$imageName,
          ]);
          //There's a few different ways to look at this which might be part of a larger conversation, 
          // so I won't touch this part yet. But is this something specific to drawings? Or is the broadcast
          // of model creation something that will happen for lots of different models? Regardless, I would 
          // change the name from NewDrawing to something like DrawingCreationBroadcast, or something like that
          // NewDrawing gets confusing since its too close to new Drawing();
          event(new NewDrawing($drawing));
          return response('Drawing created!',200);
      }
   }
  

ImageMeta class

    /** For all classes assume property declarations, and getters are included. 
    * Lean toward immutable classes where possible, so don't include setters 
    * unless it becomes necessary. Code re-use != object instance re-use.
    **/

    class ImageMeta {
        public function __construct($base64) {
            $this->image = str_replace('data:image/png;base64,', '', $base64);
            $this->imageName = 'draw'.time().'.png';
        }
    }

ImageArchiver class

    class ImageArchiver {
        private $config;
        
        public function __construct(ImageMeta $image) {
            $this->image = $image;
            $this->makeConfig();
        }
        
        private function makeConfig() {
            $this->config = [  
                'multipart' => [
                    [
                        'name' => 'file',
                        'contents' => $image->getImage()
                    ],
                    [
                        'name' => 'file_name',
                        'contents' => $imageName->getName()
                    ],
                    [
                        'name' => 'file_folder',
                        'contents' => 'drawings'
                    ],
                ],
            ]
        }
        
        //use type hinting here. Off the top of my head I don't know the parent class of guzzleClient
        public function archive($guzzleClient) {
            return $guzzleClient->post($this->config);
        }

    }

Answer 2

In this presentation about cleaning up code Rafael Dohms talks about many ways to keep code lean - like avoiding the else keyword. (see the slides here).

It is wise to avoid the else keyword - especially when it isn't needed - e.g. when a previous block contains a return statement:

   if($response->getStatusCode() >= 500){
       return response($response->getBody(), $response->getStatusCode());
   }else{
       $drawing = Drawing::create([
           'user_id'=>Auth::id(),
           'file_name'=>$imageName,
       ]);
       event(new NewDrawing($drawing));
       return response('Drawing created!',200);
   }

---

In the Frontend code, it appears that images is set to an empty string:

 data(){
   return{
       images:'',
   }
},

Yet in the listen callback it is used like an array:

       Echo.channel('channel-drawing')
       .listen('NewDrawing', res => {
           this.images.push(res.drawing);
       })  

It should be initialized as an array in data().

Is the method listen() called anywhere other than the mounted() callback? If not, then it may be simpler just to move the code from that method into the mounted() callback.