Cron job that runs a few checks across different tables in database

Question

I have been working on a cronjob that runs a few checks across different tables in my database.

The issue is that although it works, I feel like I am doing something horribly wrong in terms of the standards set out by others and also in the future for any adjustments I make.

The code below DOES work perfectly, but a foreach in a foreach seems horrible. Is there any way to tidy up what I am doing, or would it be acceptable code by a good standard?

// CRON JOB - Add notification if a build has been updated
require("../../commonlive.php"); 
require '../../functions/database-functions.php';
// select all users, and run a foreach
$query = "SELECT * FROM users";
$query_params = array(); 
try { 
    $stmt = $db->prepare($query); 
    $result = $stmt->execute($query_params);
} 
catch(PDOException $ex) 
{ 
    die("Failed to run query: " . $ex->getMessage()); 
} 
$rows = $stmt->fetchAll();
// for each user, check its user ID against the build tracking and then see when the build was updated last
foreach($rows as $row){
    $id = $row['id'];
    //echo "user id: $id, ";
    // check the build_tracking table for a row that matches the user id
    $query = " SELECT * FROM build_tracking WHERE user_id = :user_id"; 
    $query_params = array(':user_id' => $id); 
    try { 
        $stmt = $db->prepare($query); 
        $result = $stmt->execute($query_params);
        $rows = $stmt->fetchAll();
        foreach ($rows as $row) {

            $build_id = $row['build_id'];
            $user_id = $row['user_id'];
            $view_time = $row['view_time'];

            //echo "build id track: $build_id";

            // check the build_tracking table for a row that matches the user id and build id.
            $query = " SELECT * FROM blogs WHERE id = :build_id AND frontpage = 1"; 
            $query_params = array(':build_id' => $build_id); 
            try { 
                $stmt = $db->prepare($query); 
                $result = $stmt->execute($query_params);
                $rows = $stmt->fetchAll();
                foreach ($rows as $row) {
                    $build_id = $row['id'];
                    $buildOwnerID = $row['userid'];
                    $lastupdated = $row['lastupdated'];

                    $stmt = $db->prepare("SELECT * FROM followers WHERE userid = $user_id AND blogid = $build_id");
                    $stmt->execute();
                    $stmt->setFetchMode(PDO::FETCH_ASSOC);
                    $countfollow = $stmt->rowCount();

                    if ($view_time < $lastupdated && $countfollow > 0) {
                        $userTo = $id;
                        $userFrom = $buildOwnerID;
                        $type = 'buildUpdate';
                        $date = time();
                        $postID = 1;
                        $buildID = $build_id;
                        $commentID = 1;

                        // see if a notif already exists with the user id and build id
                        $query = "SELECT * FROM notifications WHERE user_to = :userTo AND buildID = :buildID AND type = :type AND status != 3";
                        $query_params = array(':userTo' => $userTo, ':buildID' => $buildID, ':type' => $type); 
                        try { 
                            $stmt = $db->prepare($query); 
                            $result = $stmt->execute($query_params);
                            $checkForExistOfNotif = $stmt->rowCount();
                        } 
                        catch(PDOException $ex) 
                      { 
                            die("Failed to run query: " . $ex->getMessage()); 
                      } 
                        // if no add a notification, if YES
                        if (!$checkForExistOfNotif) {
                            //echo "adding, $build_id";
                            addNotification($userTo, $userFrom, $type, $date, $postID, $buildID, $commentID);
                        }
                    }
                }
            }
                catch(PDOException $ex) { die();
            }
        }
    }
        catch(PDOException $ex) { die();
    }
}

Answer 1

Things that strike me as odd are that you have nested PDOExceptions and in the catch blocks all you do is die. If that is the case, why bother nesting them?

Just have one try at the top and one catch at the bottom.

You are running this as a cronjob, so if it fails with die(), how does that help anyone trying to debug what went wrong? At least pass some sort of indication as to what the error was.

I would also split your code into functions, each function should do 1 thing, then you can easily read what the top level code is doing

I am not going to re-write all your code, but here is an example of what it could start to look like if you split it into functions.

It becomes a lot more readable (maintainable) even with no comments.

try {

    $users = select_all_users();

    foreach ($users as $user) {
        $builds = select_builds_by_user($user['id']);

        foreach ($builds as $build) {
            $build_id = $build['build_id'];
            $user_id = $build['user_id'];
            $view_time = $build['view_time'];

The reason I didn't bother to re-write it all, is because I am pretty sure you could merge 3 or more of those SQL statements and using joins to fetch the data in one query, rather then the foreach, foreach, foreach going on.

For example you start off by selecting all users and then looping though them checking to see if builds exist. (I have no idea what your database structure is, so I can only offer guesses here) but this SQL selects all builds for users in one go.

 SELECT * 
   FROM build_tracking
 JOIN users ON build_tracking.user_id=users.id

Hope this gives you some of ideas of things to work on

Answer 2

You can clean up deeply nested situations like this by breaking the nested logic into functions.

foreach($rows as $row) {
    // Do something
    processBuilds($row['buildId']);
}

function processBuilds($buildId) {
    // Do stuff
    foreach($buildRows as $buildRow) {
        processBlogs($buildRow['blogId']);
    }
}

function processBlogs($blogId) {
    // Do stuff
    foreach($blogRows as $blogRow) {
        // Do more stuff
    }
}

Depending on the size of the application, it may make sense to take this one step further and take advantage of object-oriented programming.