RMH Homebase Exercises

As a way of getting used to refactoring code and rooting out "bad smells" in code, we were assigned to download and explore RMH Homebase.  Homebase is an open source volunteer scheduling system.  The code is written in php, so I decided to approach the assignment by looking at a single php file for bad smells.

I picked one of the larger files called editMasterSchedule.php and immediately found some problems with the code.  To begin with, only one of the twelve functions within the file had any sort of commenting.  Because of this, it was really hard to figure out what was going on and took some time to be able to make my own comments.  As an example using one of the shorter methods, the function do_slot_num($shift) takes in a shift as a parameter and then returns how many slots are available for that particular shift.  And that literally is what I put down for the comment.

While I was commenting the code, I also noticed that one of the functions, do_fill_vacancy_form($shift, $fam), was left as a stub and did not do anything at all.  So I removed that from the file.

Ironically, the only function that was actually commented was a long method, or a method that performs more that one task.  And it was the comments that gave it away.  The comments disclose that the function checks to see if a person is working a given shift, and also checks to see if a person is available to work any shift.  This should be two separate functions and I rewrote the code to reflect this.

The last function in the file is a really long if statement:

function get_day_names($shift,$day) {
      if($day=="Mon") {
       $shift[]= "Monday";
       $shift[]="Mon";
      }
      if($day=="Tue") {
       $shift[]= "Tuesday";
       $shift[]="Tue";
      }
      if($day=="Wed") {
       $shift[]= 'Wednesday';
       $shift[]="Wed";
      }
      if($day=="Thu") {
       $shift[]= "Thursday";
       $shift[]="Thu";
      }
      if($day=="Fri") {
       $shift[]= "Friday";
       $shift[]="Fri";
      }
      if(substr($day,0,3)=="Sat") {
       $shift[]= "Saturday";
       $shift[]="Sat";
      }
      else {
       $shift[]="Sunday";
       $shift[]="Sun";
      }
      return $shift;
     }

I decided to change this out to a switch statement:

function get_day_names($shift,$day) {
      switch ($day) {
             case "Mon":
                  $shift[]= "Monday";
                  $shift[]="Mon"; break;

             case "Tue":
                  $shift[]= "Tuesday";
                  $shift[]="Tue"; break;

             case "Wed":
                  $shift[]= 'Wednesday';
                  $shift[]="Wed"; break;

             case "Thu":
                  $shift[]= "Thursday";
                  $shift[]="Thu"; break;
    
             case "Fri":
                  $shift[]= "Friday";
                  $shift[]="Fri"; break;
   
             case "Sat":
                  $shift[]= "Saturday";
                  $shift[]="Sat"; break;
   
             default:
                  $shift[]="Sunday";
                  $shift[]="Sun";
            }
      return $shift;
     }

For me, this just looks more straightforward and readable.  Although my professor, Dr. Bowring, pointed out that this is a problem that should ultimately be solved with polymorphism.