0
\$\begingroup\$

Please check out my code below. I know I might be asking a lot, but this is newbie code which I feel can be improved quite a lot. So far I haven't been able to get it "better", or "simpler".

What does the code do?

The code first grabs all goals from the database. A goal is a simple table:

CREATE TABLE `goals` (
 `id` int(10) NOT NULL,
 `name` varchar(255) NOT NULL,
 `ratio` int(10) NOT NULL,
 `command` varchar(255) NOT NULL,
 `created_at` timestamp NULL DEFAULT NULL,
 `updated_at` timestamp NULL DEFAULT NULL
)
INSERT INTO `goals` (`id`, `name`, `ratio`, `command`, `created_at`, 
`updated_at`) VALUES
(1, 'Test Goal', 50, 'daily', '2017-01-04 08:09:12', '0000-00-00 00:00:00');

It also grabs all logs from today, again, a simple table:

CREATE TABLE IF NOT EXISTS `logs` (
 `id` int(10) unsigned NOT NULL,
 `user_id` int(10) unsigned NOT NULL,
 `action_id` int(10) unsigned NOT NULL,
 `date` datetime NOT NULL,
 `created_at` timestamp NULL DEFAULT NULL,
 `updated_at` timestamp NULL DEFAULT NULL
)
INSERT INTO `logs` (`id`, `user_id`, `action_id`, `date`, `created_at`, 
`updated_at`) VALUES
(1, 1, 1, '2017-01-02-11-22-58', '2017-01-02 06:30:43', '2017-01-02 
10:22:58', '2017-01-02 10:22:58'),
(2, 1, 2, '2017-01-02-11-23-14', '2017-01-02 07:57:59', '2017-01-02 
10:23:14', '2017-01-02 10:23:14'),
(3, 1, 4, '2017-01-02-11-23-23', '2017-01-02 08:12:09', '2017-01-02 
10:23:23', '2017-01-02 10:23:23');

Afterwards, I grab all related action_id's which are related to each goal, and put them into an array. I also grab all action_id's from all logs and put them into an array as well.

Next, I check how many action_id's are the same in both arrays. Then, I calculate the percentage based on those results, and perform an if check, and if it passes, I put a certain value into the database.

<?php
use App\Models\Log;
use App\Models\Goal;
use Carbon\Carbon;
 Route::get('test', function() {
 // Grab all goals where the command is set to "daily"
 $goals = Goal::where('command', 'daily')->get();
 // Grab all logs from today
 $logs = auth()->user()->logs()->whereDate('date', '=', Carbon::today()->toDateString())->paginate(25);
 $action_array = [];
 // Loop over all goals
 foreach($goals as $goal)
 {
 // Loop over all actions related to the given goal and put the action_id in the $action_array
 foreach($goal->actions as $action)
 {
 $action_array[] = $action->id;
 }
 }
 $log_array = [];
 // Loop over all logs and put the action_id related to each log in the $log_array
 foreach($logs as $log)
 {
 $log_array[] = $log->action->id;
 }
 // Count the total number of actions from each log from today that are equal to the goal actions
 $log_count = count(array_intersect($action_array, $log_array));
 // Loop over each goal again
 foreach($goals as $goal)
 {
 // Count the total number of actions related to each goal
 $action_count = count($goal->actions);
 // Calculate the percentage of
 $total = $action_count / $log_count * 100;
 // Check wether the total percentage of completed logs is equal to or bigger than the goal percentage
 if($total >= $goal->ratio)
 {
 // Insert a value into the database, not important for this example
 }
 }
 });
asked Jan 4, 2017 at 9:05
\$\endgroup\$
4
  • 2
    \$\begingroup\$ Can you please summarize the code's function(s) in the title? We all want to make our code better on this site, no need to ask for it. :) \$\endgroup\$ Commented Jan 4, 2017 at 9:15
  • \$\begingroup\$ Your goals table does not have a field called actions so it is unclear to me where $goal->actions is populated from or really just how goals are related to actions. Also, why paginate logs query if you are trying to compare all actions against all logs? My guess is that you can get the values you are looking for directly with a single query, but I can;t really give suggestion without understanding these two points. \$\endgroup\$ Commented Jan 18, 2017 at 19:39
  • \$\begingroup\$ Ah, the paginate seems to be an error, it shouldn't be there indeed. And the goal actions are coming from a pivot table 'goal_actions'. \$\endgroup\$ Commented Jan 19, 2017 at 12:01
  • \$\begingroup\$ @RonBrouwers You should update your answer to include information on that join(? => typically a table providing many-to-many relationship would be called a 'join' table, not a 'pivot' table, which is something entirely different and outside the domain of typical relational databases), as right now the relationship is not clear. \$\endgroup\$ Commented Jan 20, 2017 at 20:45

1 Answer 1

2
\$\begingroup\$

I think your biggest takeaway from this exercise is that, while using "natural language" query builders like those frequently included in popular frameworks represent an easy way for new developers to work with a database, they can also lead to really bad implementation decisions, especially in cases like this where you are trying to query information across a number of tables, or perform aggregate calculations that are best done in the database.

I think you should be looking to run a query like what I have shown below to get information on the percentage completion of actions for each goal. This is assuming there is an actions table related to goals as noted in your comments.

SELECT
 goals.id AS goal_id,
 goals.name AS goal_name,
 COUNT(actions.id) AS action_count,
 COUNT(logs.actions_id) AS completed_action_count,
 ((completed_action_count / action_count) * 100) AS percentage_complete
FROM goals
INNER JOIN actions
 ON goals.id = actions.goal_id
LEFT JOIN logs
 ON actions.id = logs.action_id 
GROUP BY
 goal_id
ORDER BY
 goal_id ASC /* you could obviously order by whichever field you are interested in */

This will save you a lot of juggling of information in your PHP application, as you are now fully leveraging the relational and aggregation capabilities of the database.

If you ever find yourself trying to pull contents from two related tables into your application in order to compare them, this should be a red flag to you that you might need to look at using your framework's join or raw SQL query capability (they pretty much all have this), in order to execute your query.

You could potentially even go a step further and go directly to updating your database as well based on the results for this query. You could add a WHERE percentage_complete >= ? clause here to further limit the result set to those where you completion ratio is what you desire and perhaps use INSERT ... SELECT syntax to directly insert information from this query into an appropriate table.

Don't be afraid to venture forth into better understanding the full capabilities of SQL. There are a number of on-line resources like StackOverflow that can help you out if you are having problems getting a query to work.

answered Jan 20, 2017 at 21:10
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.