4
\$\begingroup\$

The following script is being run with a cron. It runs once a minute to see if there is a reminder that needs to be sent.

Should I make this run once every 30 minutes? The choices to send a reminder are, up to 12 hours:

  • 30 minutes
  • 60 minutes
  • 90 minutes
  • 2 hours

Could this script be optimized/written differently?

 <?php
    require_once ("connect.php");
    require_once ("function.php");
    $sql = "Select
      signings.signstart,
      signings.starttime,
      remindertime.rtime,
      signings.id,
      signings.fname,
      signings.lname,
      signings.street1,
      signings.street2,
      signings.city,
      signings.zip,
      signings.hphone,
      signings.cphone,
      signings.email,
      signings.coworkphone,
      signings.coname,
      signings.cocellphone,
      profile.timez,
      profile.email As email1,
      profile.cell,
      profile.textmsg,
      profile.emailORmessage,
      profile.emailmsg,
      profile.servid
    From
      signings 
      Inner Join
      profile On signings.pid = profile.id 
      Inner Join
      remindertime On remindertime.remiderid = profile.remiderid
    Where
      signings.signstart = CurDate() And
      signings.done = 0";
    foreach ($db->query($sql) as $stm) {

        $newsub = $stm['rtime']; // the time that needs to subtracted from the scheduled date this comes from the remindertime table 30 minutes 1 hour 2 hour ..etc 

        $datetrue = $stm['signstart'] . ' ' . $stm['starttime']; //The schedule appointment time

        $dateTime = new DateTime($datetrue, new DateTimeZone($stm['timez'])); //DateTimeZone is the timezone that the User is working in America/Los_Angeles or America/New York ...etc
        $senddate1 = $dateTime->format("Y-m-d H:i"); //the date in the selected timezone
        $date = date_create($senddate1);
        $date1 = date_sub($date, date_interval_create_from_date_string($newsub)); //subtract the amount of time before the appontment time you  want the reminder sent
        $senddate = date_format($date1, 'Y-m-d H:i'); //the datetime to send the reminder


        if ($senddate == date('Y-m-d H:i')) { // if the date is equeal to the date and time right now then send the reminder

            //get all the customers data to put in the message
            echo $pid = $stm['pid']; // the agents id number


            $emailORmessage = $stm["emailORmessage"];

            if ($emailORmessage == 1) { //if 1 then send cell phone message
                $textmsg = $stm["textmsg"]; // if textmsg is 1 yes send message
                if ($textmsg == 1) {
                    $servid = $stm["servid"];
                    $domname = cellserver($servid, $db);
                    $order = array(
                        "(",
                        ")",
                        " ",
                        "-");
                    $replace = '';
                    $cell = str_replace($order, $replace, $stm["cell"]);
                    $emailaddress = $cell . $domname;

                    //Now send message
                    $signingid = $stm['id'];
                    $to = $emailaddress;
                    $subject = 'Reminder';
                    $message = cellmessage($signingid, $db, $datetrue);
                    $headers = 'From: rdkurth@gmail.com' . "\r\n" . 'Reply-To: rdkurth@gmail.com' .
                        "\r\n" . 'X-Mailer: PHP/' . phpversion();

                    mail($to, $subject, $message, $headers);
                } //end if textmsg = 1


            } else { //send email
                $emailmsg = $stm["emailmsg"]; // if emailmsg is 1 yes send email

                if ($emailmsg == 1) {
                    $email1 = $stm["email1"];

                    //Send the email


                    $to = $email1;
                    $subject = 'Appointment Reminder';
                    $message = emailmessage($signingid, $db, $datetrue);
                    $headers = 'From: rdkurth@gmail.com' . "\r\n" . 'Reply-To: rdkurth@gmail.com' .
                        "\r\n" . 'X-Mailer: PHP/' . phpversion();

                    mail($to, $subject, $message, $headers);
                } //end if emailmsg = 1

            }
        } //end of if senddate =
    } //end foreach


    ?>
\$\endgroup\$
1
\$\begingroup\$

Yes, it could be written very differently. You could write this in an OO style. Use a factory to determine the type of reminder to send (email/sms), then call the send method on the reminder. Create a daemon process by using a cron to kickstart it every 60 seconds. If it detects another process running, don't start a second process. This means you can then check the db as little or as often as you like and you don't have to mess with the cron for deployments (since the same kick starter script can start any number of additional daemons).

For all repeated queries it's good practice to "prepare" and "execute" them. The second time around they would already have been compiled by the db so saves execution time the second time around.

Ok, so the best way to explain this to at least get you on the rights tracks structurally is to give you some pseudo code. I've tried commenting it to help you follow it, but it is only pseudo code and scaffolding at that. You will have to fill in the details:

/**
* Remember to set things like flood control if the daemon doesn't do any work, you don't want to kill the db that has the registered heartbeats.
* Set a maximum execution time
* Set a maximum iterations limit
* Set iteration rate (so it sleeps for a microsecond before starting next iteration so it doesn't flood any resources)
* Etc
*/
abstract class Daemon {

    /**
    * Will see if any other daemons are registered and if so, kill itself - must be stored in something distributed such as the DB, memcache or redis.
    */
    public function __construct() {

    }

    /**
    * What the daemon will actually do
    */
    abstract function execute();

    /**
    * Every iteration calls this to check if the daemon should continue executing
    */
    protected function isAlive() {

    }

}

/**
* Actually send the reminders
*/
class DaemonReminders extends Daemon {

    /**
    * @override
    */
    public function execute() {

        // Do you sending in here...
        while ($this->isAlive()) {

            // Check if anyone need a reminder
            $remindersRequired = ...;

            // If so, create relevant reminder using the factory
            $reminderFactory = new ReminderFactory();
            $reminderFactory->setEmailAddress($remindersRequired->emailAddress);
            $reminderFactory->setMobileNumber($remindersRequired->mobileNumber);
            $reminderMethod = $reminderFactory->create();

            // Send reminder
            if ($reminderMethod->send()) {

                // Sent ok, so update the reminders record
                ...

            }

        }

    }

}

class ReminderFactory {

    /**
    * Pass in the required values used to determine the type of reminder object to create
    */
    public function __construct() {

    }

    /**
    * Builds and returns the relevant Reminder object
    */
    public function create() {
        if (...) {
            return new ReminderSMS();
        }
        if (...) {
            return new ReminderEmail();
        }
        throw new Exception("Unknown reminder type...");
    }

}

abstract class Reminder {

    /**
    * Send the reminder
    */
    abstract public function send();

}

class ReminderSms extends Reminder {

    public function __construct($smsNumber) {

    }

    /**
    * @override
    */
    public function send() {

    }

}

class ReminderEmail extends Reminder {

    public function __construct($emailAddress) {

    }

    /**
    * @override
    */
    public function send() {

    }

}

// daemon-reminders.php <- cron set to every 60 seconds

try {
    $daemon = new DaemonReminders();
    $daemon->execute();
    catch (Exception $e) {
    $daemon->shutdown();    // properly unregister the daemon so it can start again next time (doesn't have to wait for old heartbeat to be considered dead)
    throw $e;           // So error gets logged
}

Comment if you have any questions.

\$\endgroup\$
  • \$\begingroup\$ I thank you for your example but I am not very good with classes I wright in Procedural style that is how I learned and have not been able to learn classes. I am only running the cron every 30 min now and it seams to work \$\endgroup\$ – Richard Kurth Jun 19 '14 at 20:46
  • \$\begingroup\$ You will get problems if they ever overlap. As in if your script takes longer to run than 30 mins. Remember, max execution time does NOT include time waiting for blocking connections (such as DB queries taking a while). So be careful. \$\endgroup\$ – HenchHacker Jun 19 '14 at 21:34
0
\$\begingroup\$

I don't know how important everything is, but ideally, you don't need to check everything every minute, and even if you do, you could 'queue' up a cron job from another cron.

For instance

  • cron-1 runs every week and moves reminders that need to run into a reminder file.
  • cron-2 runs once a day and moves reminders that need to run into a reminder file for cron 3.
  • cron-3 runs once an hour and moves reminders into a file for cron 4.
  • cron 4 runs once a 'minute' as you have and runs from file 4.

This would be 'more' efficient, but what I would do is just queue everything up for the hour, and use JavaScript as a timer.

IE Cron1->Reminder for 6/16->move to file->cron2->reminder for 6/19->move to file->cron 3->reminder for 6:30pm->que for web page->

javascript_timer_function(x,y, z)
{
    [get the hour and figure out how many more milliseconds]
    window.setInterval("alert('.z.')", y);
} 
\$\endgroup\$
  • \$\begingroup\$ I have changed it to run every 30 min because the smallest time to send a reminder before the appointment in 30 min \$\endgroup\$ – Richard Kurth Jun 19 '14 at 20:41

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.