trjhtr

I write small inboxing app in Laravel 5.5 for my wesite. In inbox app users can compose new message, read messages and delete messages. I have only one table in my database for inboxes.

Structure of messages table:

And I have model Messages for this table.

Code: InboxController

<?php

namespace AppHttpControllers;

use AppUser;
use AppMessage;
use IlluminateHttpRequest;
use IlluminateSupportFacadesAuth;
use IlluminateContractsEncryptionDecryptException;

class InboxController extends Controller

 ## Inbox View Control Methods ##

 // View for show new messages of current user

 public function messages()
 
 $array = array
 (
 'messages'=>$this->user_messages('my_messages'), 
 'count'=>$this->count_messages()['my_messages'],
 'count_sended' => $this->count_messages()['sended_messages'],
 );

 return view('index', $array);
 

 // View for show sended messages of current user

 public function sended()
 
 $array = array
 (
 'messages'=>$this->user_messages('my_sended_messages'), 
 'count'=>$this->count_messages()['my_messages'],
 'count_sended' => $this->count_messages()['sended_messages'],
 );
 return view('sended', $array);
 

 // View for show trashed messages of current user

 public function trashed()
 
 $array = array
 (
 'messages'=>$this->user_messages('my_sended_messages'), 
 'count'=>$this->count_messages()['my_messages'],
 'count_sended' => $this->count_messages()['sended_messages'],
 );
 return view('sended', $array);
 

 // View for compose and send new message

 public function compose(Request $request)
 
 if($request->isMethod('post'))
 

 // Sended data from html form 
 $to = $request->to;
 $from = Auth::user()->id;
 $subject = $request->subject;
 $message = $request->message;

 // Get info about receiver user
 $receiver_user_id = User::where('email', $to)->first()->id;
 $receiver_user_type = User::where('id', $receiver_user_id)->first()->type; 


 // Get info about sender user 
 $sender_user_type = Auth::user()->type;

 // Send message
 Message::create(
 [
 'subject'=>$subject, 
 'message'=>$message, 
 'from'=>$from, 
 'to'=>$receiver_user_id
 ]
 );

 

 $count = $this->count_messages()['my_messages'];
 $count_sended = $this->count_messages()['sended_messages'];

 $array = array
 ( 
 'count' => $count,
 'count_sended' => $count_sended,
 );

 return view('compose', $array);
 

 // View for read message

 public function message(Request $request, $id)
 
 try 
 
 $decrypted = decrypt($id);
 

 catch (DecryptException $e) 
 
 return redirect()->back();
 


 $user_id = Auth::user()->id;

 $messages = Message::where('id', decrypt($id))->get();
 dump($messages);
 $message_to_id = $messages->first()->to;
 if($user_id == $message_to_id)
 
 $subject = $messages->first()->subject;
 $message = $messages->first()->message;
 $status = $messages->first()->status;
 $date = $messages->first()->created_at;

 $sender_id = $messages->first()->from;
 $sender = User::where('id', $sender_id)->first()->name;
 $count = $this->count_messages()['my_messages'];
 $count_sended = $this->count_messages()['sended_messages'];

 $array = [
 'subject' => $subject,
 'message' => $message,
 'date' => $date,
 'messages'=> $messages,
 'sender' => $sender,
 'status' => $status,
 'count' => $count,
 'count' => $count,
 'count_sended' => $count_sended,
 ];

 if($status == 0 && $sender_id !== $user_id)
 
 Message::where('id', decrypt($id))->update(['status' => 1]);
 

 return view('message', $array);
 

 else return redirect()->back();

 

 ## Helper functions for get any info from messages and users table ##

 public function count_messages()
 
 $user_id = Auth::user()->id;
 $my_messages = Message::where('to', $user_id)->where('status', 0)->count();
 $sended_messages = Message::where('from', $user_id)->count();
 $array = ['my_messages' => $my_messages, 'sended_messages' => $sended_messages];
 return $array;
 

 public function user_messages($status = 'my_messages')
 
 $user_id = Auth::user()->id;

 switch ($status) 
 
 case 'my_messages':
 $messages = Message::where('to', $user_id)->orderBy('created_at','desc')->get();
 break;
 case 'my_sended_messages':
 $messages = Message::where('from', $user_id)->orderBy('created_at','desc')->get();
 break;
 default:
 $messages = false;
 break;
 

 return $messages;
 

How I can reduce code duplication and beautify my controller code?

Reduce Complexity, Follow SRP

The Single Responsibility Principle states that every module or class should have responsibility over a single part of the functionality provided by the software, and that responsibility should be entirely encapsulated by the class. All its services should be narrowly aligned with that responsibility.

Robert C. Martin expresses the principle as follows:
A class should have only one reason to change.

While this is primarily targeted at classes in object oriented languages it applies to functions and subroutines well.

The public function message(Request $request, $id) function could be broken up into at multiple functions, especially the contents of the if($user_id == $message_to_id) block.

The more separate functions there are the easier it is to understand or read the code. This also makes it easier for any programmer to maintain or debug the code.

Don't Repeat Yourself

In software engineering, don't repeat yourself (DRY) is a principle of software development aimed at reducing repetition of software patterns, replacing them with abstractions; and several copies of the same data, using data normalization to avoid redundancy.

Generally when there is repeating code in a software module it indicates that a function should be written to contain that code or a loop should be written to perform the repetition.

When code repeats in different functions it becomes a maintenance problem. Someone can fix the code in one location and miss it in another location. The solution to this is to write a function for the code that repeats.

• stop using SQL builders in your "controllers"


• stop rendering templates in your "controllers"


• stop repeating authentication in each class method


• stop using static function calls and relaying on global state

Though, I think all those are "laravel best practices" ... hmmm ... I wonder, what conclusions that might lead to.

You can move all your business logic into services. I also recomend you to Use repository pattern in order to reduce number of queries. And finally use dependency injection || Service Container.

Here the link for Service Container tutorial.
You can also create seperate class for handling your business logic and then inject them into controllers __constructor. Then you will endup with less than 10 lines of code.