One thing that you should not do is process data inside of your routes.php.
This is something called "separation of concerns".
There's a few things we can do to clean up this code a bit here:
Keep your 'getMatrix' method where it is (in your Model I assume).
Create a controller to handle the data that comes through, make sure to name it accordingly, but for the sake of example, we will call it the 'MatrixController':
class MatrixController extends \BaseController {
public function getDashboardIndex() {
$invoice = new \Invoice();
$invoice->getMatrix();
$invoices = $invoice->all()->count();
$approved = count($invoice->approved());
$inProcess = count($invoice->where('assigned_to', '!=', 0)->get());
$deleted = count($invoice->deletedInvoices());
$paid = count($invoice->paid());
$lastUpdated = File::lastModified('dashboard.json');
return View::make('dashboard')
->with('invoices', $invoices)
->with('deleted', $deleted)
->with('approved', $approved)
->with('paid', $paid)
->with('deleted', $deleted)
->with('inProcess', $inProcess)
->with('data', $data)
->with('lastUpdated', $lastUpdated);
}
}
Then we can change your route to:
//route to the dashboard
Route::get('dashboard', [
'uses' => 'MatrixController@getDashboardIndex',
'as' => 'dash.index'
]);
//let's make sure we can see our Restful controller
Route::controller('dashboard', 'MatrixController');
Let me know if this helps out or makes things faster.
I believe that the practice mentioned by ayyobro is good, but it will not reduce the execution time.
From your code it is obvious that you are not aware of the action going on when you call a simple method like get() in Laravel.
All your data is stored in the database, and you have to ask the database to give you the needed information. This is a slow process, because the database has to check who you are, what do you want, do you have the needed permissions, do that things you are asking for exist, go throug those tables, find matches, make a response. Then the server decodes that response, makes a new model.... Really a lot of stuff. Try making as less queries as possible.
If you need some value multiple times, store it!
public static function getMatrix(){
$matrix = array();
$maxWeight = Designation::where('name', '=', 'Disbursing Officer')->first()->weight;
foreach(Designation::orderBy('weight', 'ASC')->get() as $designation){
if($designation->weight <= $maxWeight ){
This alone will speed your algorithm up.
If I'm right, your view needs only the count of Invoices in different states. If yes, then don't ask the database for all the invoices all over again, istead ask it for the count. That is what SQL is for. The count() function makes the needed query for you.
$invoices = Invoice::count();
$approved = Invoice::approved()->count();
$inProcess = Invoice::where('assigned_to', '!=', 0)->count();
$deleted = Invoice::deletedInvoices()->count();
$paid = Invoice::paid()->count();
$lastUpdated = File::lastModified('dashboard.json');
I hope those approved, paid and so on are query scopes.
OMG!
if you order those Designations by weight, break when you find the first one over the maxValue.
public static function getMatrix(){
$matrix = array();
$maxWeight = Designation::where('name', '=', 'Disbursing Officer')->first()->weight;
foreach(Designation::orderBy('weight', 'ASC')->get() as $designation){
if($designation->weight <= $maxWeight ){
$matrix[$designation->weight] = array();
...
Change it to
public static function getMatrix(){
$matrix = array();
$maxWeight = Designation::where('name', '=', 'Disbursing Officer')->first()->weight;
foreach(Designation::orderBy('weight', 'ASC')->get() as $designation){
if($designation->weight > $maxWeight )
break;
$matrix[$designation->weight] = array();
...
It becomes even better! Instead of retrieving all that Designations and looking which one is over the maxValue, just ask the database to retrieve only the ones wich are under.
public static function getMatrix(){
$matrix = array();
$maxWeight = Designation::where('name', '=', 'Disbursing Officer')->first()->weight;
foreach(Designation::where('weight', '<=', $maxWight)->orderBy('weight', 'ASC')->get() as $designation){
$matrix[$designation->weight] = array();
...
Worst code ever!
Please, use local variables
foreach(Designation::where('weight', '<=', $maxWight)->orderBy('weight', 'ASC')->get() as $designation){
$myLocalVar = array();
$myLocalVar[...] = ...
...
$matrix[$designation->weight] = $myLocalVar;
Don't make that orderBy if you don't need it. It slows you down.
Thanks Ayyobro and Firtzberg for you quick reply, I agree with ayyobro that making a controller and then calling it fron routs via named routes is corrent way of working but the code you are seeing here was an attempt of making the execution fast. I thought I can make the code run faster if I keep it in route instead of routing through the controller. I dont think it will help my cause.
And for Firtzberg thank you for pointing out bad practices. I will try making changes to code, and run then will let you know if that helps.
Please let me know any other way I can make it run faster Thanks
I forgot to mention that On first place I used the controller and then switch to this code in order to make execution fast.
Sign in to participate in this thread!
The Laravel portal for problem solving, knowledge sharing and community building.
The community