2
\$\begingroup\$

I've only been writing php for a couple of months, and I've never really had anyone to look at any code I have written. I've written this class, that returns an email address from a database, based on a set schedule. I feel like a lot of the time, I'm doing things the long way, or just the wrong way. So I would really appreciate it if someone could review this class, and make any suggestions at all, as far as coding style, optimization, etc..

Let me explain how it works - a quick overview.This email script is called by procmail, and the email address it returns gets forwarded by procmail. The database it connects to has 4 tables(right now).

The schedule table has 4 columns, and time in/out are military time.

date | tc_name | time_in | time_out

The Tour Consultant table has columns:

tc_name | friendly_name | email

The counters table holds the counters for the currently active Tour Consultants:

tc_name | count

The lastactivehash table is one field only, and just holds a string of the
last tour consultants that were active. Each time this string changes the counters
table is flushed, and reinitialized with the current active Tour Consultants.

How this is supposed to work: Should divy up emails evenly between current active Tour Consultants. If no one is active, it should look ahead to tomorrow, if it is evening, or the same day if it is morning, and process everyone that is working that day.

I tried to name the functions in a way that would explain what they are doing. This is a pretty simple class, but if I should comment it out, let me know.

Note: get_manual_override is not implemented.

<?php


class schedule {

private $tomorrow;
private $today;
private $timenow;
private $dblink;
private $active_day;
private $active_time;
private $counters;
public $final_email;

  function __construct() {
       $this->dblink = new mysqli('127.0.0.1', '####', '####', '####');
       date_default_timezone_set('America/Anchorage');


      $this->today = date('Y-m-d');

       $this->tomorrow = date('Y-m-d', mktime(0, 0, 0, date("m")  , date("d")+1, date("Y")));
     //military time
       $this->timenow = date('Hi');
     //This is to be implemented as of yet
    if ($address = $this->get_manual_override){
       //email it
       die();
    }
    else{
          //Query the db for everyone that has today's date set
       if ($this->get_active_full_day($this->today)){
          //Filter these results based on who is active at the current time.
             $this->get_active_time();

       }
         //Fallback, try to get tomorrow.
       else{
         if($this->get_active_full_day($this->tomorrow)){
         $this->process_whole_day();
         $this->filter_active();
         }
         //Ultimate Fallback, set a default array of emails here (TODO)
         else{
        echo 'didnt find anything';
         }
       }
    }

  }

   //Query the db for people who are active today
    private function get_active_full_day($date) {

   $query = "SELECT * FROM schedule LEFT OUTER JOIN tour_consultants ON tour_consultants.tc_name = schedule.tc_name 
   WHERE `date` = '$date'";

  $result = $this->dblink->query($query) ;
 if((isset($result->num_rows)) && ($result->num_rows != '')) {
    $itr = 0;
    //Store the results into an associative array.
  while ($row = $result->fetch_assoc()) {
        $this->active_day[$itr]['time_in'] = $row['time_in'];
        $this->active_day[$itr]['time_out'] = $row['time_out'];
        $this->active_day[$itr]['tc_name'] = $row['tc_name'];
        $this->active_day[$itr]['email'] = $row['email'];
        $itr++;
    }



    return true;
    }
    else{
    return false;
    }
  }

 //This will only run if Today's date is set up in the database.
    private function get_active_time() {
     //Loop through the array of active today, and look for people who are currently working.
     //If they are active, add them to the activetime array.
    foreach($this->active_day as $record => $ar) {

      if($this->is_between($this->timenow, $ar['time_in'], $ar['time_out']))
        $this->active_time[] = $ar; 

    }
//If it didn't find anybody currently active.
    if(!isset($this->active_time)){
     $times_out = array();
     $times_in = array();
//Make an array of everybody working today's times in and times out.      
     $itr = 0;
      foreach($this->active_day as $record => $ar) {
        $times_in[$itr] = $ar['time_in'];
        $times_out[$itr] = $ar['time_out'];
        $itr++;
      }
//If the time now is less than the minimum of the times in, then it is morning, and process everyone working 
    //Today         
      if($this->timenow < min($times_in)) {
        if($this->process_whole_day()){
          $this->filter_active();
        return true;
        }
        else{
            return false;
        }
  //If the time now is later than the max of times out, get everyone working tomorrow, and process them.
      }
      elseif($this->timenow > max($times_out)) {
        if($this->get_active_full_day($this->tomorrow)){
           if($this->process_whole_day()){
            $this->filter_active();
            return true;
           }
           else{
            return false;
           }
      }
      else{
        return false;
      }



    }
    else {
        //THis else happens if we are probably between shifts...
        //Process the whole current day here.
        $this->process_whole_day();
            $this->filter_active();
    }

    }
        //This else is what happens when it does find people working at the current time.
    else{
        $this->filter_active();
        return true;
    }

  }





 private function filter_active() {
    if(!isset($this->active_time)){
        return false;
    }
    else{

/* Get a list of the names of people that were active last time an email was sent.  If the list has changed, reset the email counters, if it hasn't changed, get the current counters. */   
if($lastactive = $this->get_last_active()){

    $curractive = '';
    foreach($this->active_time as $arr) {
    $curractive .= $arr['tc_name'];
       }


    if($lastactive != $curractive) {
        $this->reset_counters();


    }
    else{
        $this->counters = $this->get_counters();
    }
}
 //Error getting last hash, so reset counters to be safe.
else{
    $this->reset_counters();
}

 /* Add the counters array to the active time array. */ 
$min = min($this->counters);
        foreach($this->active_time as $id => $arr) {
            if(isset($this->counters[$arr['tc_name']])){
            $this->active_time[$id]['sent'] = $this->counters[$arr['tc_name']];
            }
            else{
                $this->active_time[$id]['sent'] = 0;
                $min = 0;
            }


        }

    /* Find the people who have been emailed the least */   
        foreach($this->active_time as $id => $arr) {
            if($arr['sent'] == $min){
                $leastsent[$id] = $arr;
            }


        }
/* If more than one person has the same minimum counter, pick a random one of them. */ 
        if(count($leastsent) > 1){
            $final = array_rand($leastsent, 1);
            $final = $leastsent[$final];
        }
        else{   
            $final = $leastsent['0'];
        }

        if(isset($final)) {

             $newcounter = $final['sent'];
        /* Increment the counter, and store it in the database, then set the lastactive names in the database. ($this->activetime) */
                            $newcounter++;
                $this->set_counter($final['tc_name'], $newcounter);
            $this->set_last_active();

            $this->final_email = $final['email'];

        }




    }


 }






 /* Get the list of people who were last active */ 
     private function get_last_active() {
     $query = "SELECT hash FROM lastactivehash WHERE `id` = '0' LIMIT 1";

     if($result = $this->dblink->query($query)) {

      while ($row = $result->fetch_assoc()) {
       $oldhash = $row['hash'];
       }
       return $oldhash;
     }
     else{
      return false;
     }
   }

  /* Set the list of people who were active this time around */ 
   private function set_last_active() {
    $names = '';
   foreach($this->active_time as $arr) {
    $names .= $arr['tc_name'];
   }


    $query = "UPDATE lastactivehash SET `hash`='$names' WHERE `id`='0'";

    $result = $this->dblink->query($query);

      if($this->dblink->affected_rows != 1)
    return false;
    else
    return true;

   }


   /* Get the list of email counters */ 
    private function get_counters() {
    $query = "SELECT * FROM counters";

     if($result = $this->dblink->query($query)) {

      while ($row = $result->fetch_assoc()) {
       $counters[$row['tc_name']] = $row['count'];

      }
     return $counters;
     }
     else{
        return 0;
     }
   }

  /* Set a single email counter */   
   private function set_counter($name, $count) {
    if($name == '' || $count == ''){
        return false;
    }
    else{


     $query = "UPDATE counters SET `count`='$count' WHERE `tc_name`='$name'";
     $this->dblink->query($query);

    if($this->dblink->affected_rows != 1)
    return false;
    else
    return true;

    }

    }

   /* Reset email counters, set everybody with an active time to 0 */   
    private function reset_counters() {
        $truncate = "TRUNCATE TABLE counters";
        $this->dblink->query($truncate);

        foreach($this->active_time as $arr){
            $name = $arr['tc_name'];
            $query = "INSERT INTO counters (tc_name, count) VALUES ('$name', '0')";
         $this->dblink->query($query);
         if($this->dblink->affected_rows != 1)
         $bad = 1;
        $this->counters[$arr['tc_name']] = '0';
        }

        if ($bad = 1)
         return false;
        else
        return true;
    }

 /* Simple utility function check if one value is between two others. */
    private function is_between($value, $min, $max){

         if (($value >= $min) && ($value <= $max))
           return true;
         else
           return false;
    }


  /* Helper function - Set the array of active_day, to currently active, active_time */  
    private function process_whole_day(){

    if((isset($this->active_day)) && (!isset($this->active_time))){
    $this->active_time = $this->active_day;
    return true;
    }
    else{
    return false;
    }

    }




}
\$\endgroup\$
1
  • \$\begingroup\$ Ok, I went through and commented out the whole mess. Will someone give me some suggestions please? \$\endgroup\$
    – Ben
    Jul 7, 2011 at 17:11

1 Answer 1

4
\$\begingroup\$

I'd consider:

  • breaking it into smaller object (e.g. extract counters)
  • do not hardcode the dblink connection + settings (e.g. pass the object in the constructor)
  • use phpdoc comments
  • correct formatting

Try to write unit test for this class, you'll spot all the drawbacks quickly.

\$\endgroup\$

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.