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


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 ?

  • \$\begingroup\$ DbSql is a database abstraction layer that you developed yourself? \$\endgroup\$ – 200_success Apr 6 '17 at 11:26
  • \$\begingroup\$ no, it uses eloquent laravel outside laravel \$\endgroup\$ – Robert Brax Apr 6 '17 at 11:26

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?


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?

| improve this answer | |

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.