Sending email notifications, each to a different set of subscribers

Question

Here is a code which sends email notification on new comments.

<?php
namespace Rib\Src\Apps\ContentNotification\ContentNotificationControllers;
use Rib\Src\Apps\ContentNotification\Email\NewContentNotificationEmail;
use Rib\Src\Services\DbSql;
use Rib\Src\Services\Email;
class JobSendNotificationsCron
{
 public function sendEmails()
 {
 $db = ( new DbSql() )->db()->getConnection();
 # Collect jobs to process
 $jobsToProcess = $db->table( 'content_notifications_jobs' )
 ->where( 'status', '=', 'active' )
 ->orderBy( 'last_mailing_time', 'asc' )
 ->limit( 5 )
 ->get()->toArray() ?? null;
 # Treat each job indivicually....
 foreach ( $jobsToProcess as $job ) {
 # Get all users that subscribed to be notified about new content in a particular job
 $usersToNotify = $db->table( 'content_notifications_subscribers' )
 ->where( 'content_id', '=', $job->content_id )
 ->get()->toArray() ?? null;
 # ... and send notification to each user subscribed to the job
 foreach ( $usersToNotify as $user ) {
 # Send email confirmation email
 Email::getMailer()->send( NewContentNotificationEmail::setContent( $job, $user ) );
 }
 # Update job table. Set job to inactive so it is not treated if no new answer was posted.
 # And set last_mailing_time to also skip jobs that had a mailing before delay was expired.
 $db->table( 'content_notifications_jobs' )->update(
 [
 'status' => 'inactive',
 'last_mailing_time' => time()
 ]
 );
 }
 }
}

I don't like this code because there are sql calls in loop. I'm thinking of refactoring this code. What would be the best way to refactor it by moving the sql out of the loop ?

• 200_success: DbSql is a database abstraction layer that you developed yourself?
• Robert Brax: no, it uses eloquent laravel outside laravel

Answer 1

Is there any reason for this to be a concrete class/method?

---

You only seem to be considering happy path.

What happens in DB connection failure occurs? Have you considered passing the DB connection/object as a dependency to this class or method such that this class/method is guaranteed it gets a connection in a proper state?

---

Have you considered using a join to get notification/subscriber information in a single query and not have to query in loops?

---

Email::getMailer()->send(...)

Do you really need to instantiate mailer here every time? Should mailer be a passed dependency?

---

Do you not need a unique key for performing your update? Right now it looks like you are updating every record on the table.

---

Do you really want to be updating unix timestamps into the table as opposed to using appropriate MySQL datetime or timestamp fields?