Refactoring help...

Not for 'how-to' coding questions but PHP theory instead, this forum is here for those of us who wish to learn about design aspects of programming with PHP.

Moderator: General Moderators

Post Reply
User avatar
nielsene
DevNet Resident
Posts: 1834
Joined: Fri Aug 16, 2002 8:57 am
Location: Watertown, MA

Refactoring help...

Post by nielsene »

OK, so I've been using TDD (see all my posts in the Unit Test forum) to drive the development of some list formatting classes. These are classes that take a RecordSet and then output a list in a variety of formats. The different classes take ResultSet with different row headers (ie different columns from the DB) and apply the various business logic to the formatting.

After the second class I knew I had substantial code duplication, but didn't see a good way to remove it. I've finished the next pair of classes, hoping to triangulate in on the proper refactoring -- I'm almost positive that there's Strategy or two in here...

There's now four classes all have two methods: formatAsTabbed and formatAsText. (They are likely to gain an formatAsXML and a formatAsLaTeX. I don't expect them to grow past those four total).

The formatAsTabbed are the most similar; I think I can fold most of it up to a superclass.

The formatAsText have enough difference that I'm not sure how to refactor the common elements: the two Competitor based lists need to use look-back to combined matching entries, the CompetitorAffil needs to add subheaders, the EventEntry one needs to use slightly crazy line breaking algorithms compared to the others. Not to mention the knowledge of the different "template" for the output (but that's the esiest piece to addresss). Finally the EventList one needs to lookup data and call the EventEntry to finish its work.

Is there a good way to start unifying these?

Here's the current production code for the classes: [I apologize for the lousy indenting, looks like the PHP tag is eating tabs but not spaces or vice versa)

Code: Select all

<?php
  /**
   * Format lists of competitors into end-user friendly formats.
   * This file is part of CompInaBox.
   * @copyright CompInaBox Copyright 2001-2005. Eric D. Nielsen, All rights reserverd.
   * @license http://opensource.org/licenses/gpl-license.php GNU Public License
   *
   * @author Eric D. Nielsen <nielsene@alum.mit.edu>
   * @package DataFormatting
   * @filesource
  */
  /**
   * Formats QueryResults for use in outside documentation.
   *
   * This class handles formatting list of competitors into the
   * various layouts needed for people assembling a printed program
   * offering choices such as tabbed delimited or pre-formatted text and
   * can group the lists by affiliations or not.
   * @package DataFormatting
   */
class CompetitorListFormatter {
  function CompetitorListFormatter() {
  }

  /**
   * Return the list as tab delimited entries
   *
   * This format is designed for use by authors who wish to
   * do significant data maniupulation on the output.
   * @param QueryResult $qr A QueryResult returned from a call to DB::query()
   */
  function formatAsTabbed($qr) {
    $str = &quote;LIST OF COMPETITORS\n&quote;;
    $numRows=$qr->numRows();
    if (0==$numRows)
      $str.=&quote;none registered\n&quote;;
    else {
      for ($i=0;$i<$numRows;$i++) {
	list($num,$lfname,$llname,$laffil,
	     $ffname,$flname,$faffil) = $qr->getRowAt($i);
	$str.=&quote;$num\t$lfname\t$llname\t$laffil\t$ffname\t$flname\t$faffil\n&quote;;
      }
    }
    return $str;
  }

  /**
   * Return the list as a pre-formatted text list
   *
   * This format is designed for use by authors who wish to
   * simply drop the output directly into thier printed program with
   * little to no modifications.
   * @param QueryResult $qr A QueryResult returned from a call to DB::query()
   */
  function formatAsText($qr) {
    $str = &quote;List of Competitors, by Competitor Number\n&quote;;
    $numRows=$qr->numRows();
    if (0==$numRows)
      $str.=&quote;none registered\n&quote;;
    else {
      $storedNum=-1;
      for ($i=0;$i<$numRows;$i++) {
	list($num,$lfname,$llname,$laffil,
	     $ffname,$flname,$faffil) = $qr->getRowAt($i);
	if ($storedNum!=$num) {
	  $storedNum=$num;
	  $str.=&quote;$num $lfname $llname ($laffil) & $ffname $flname ($faffil)\n&quote;;
	} else {
	  $str.=&quote;\t\t& $ffname $flname ($faffil)\n&quote;;
	}
      }
    }
    return $str;
  }
}
?>

Code: Select all

<?php
  /**
   * Format lists of competitors into end-user friendly formats.
   * This file is part of CompInaBox.
   * @copyright CompInaBox Copyright 2001-2005. Eric D. Nielsen, All rights reserverd.
   * @license http://opensource.org/licenses/gpl-license.php GNU Public License
   *
   * @author Eric D. Nielsen <nielsene@alum.mit.edu>
   * @package DataFormatting
   * @filesource
  */
  /**
   * Formats QueryResults for use in outside documentation.
   *
   * This class handles formatting list of competitors into the
   * various layouts needed for people assembling a printed program
   * offering choices such as tabbed delimited or pre-formatted text.
   * @package DataFormatting
   */
class CompetitorAffilListFormatter {
  function CompetitorAffilListFormatter() {
  }

  /**
   * Return the list as tab delimited entries
   *
   * This format is designed for use by authors who wish to
   * do significant data maniupulation on the output.
   * @param QueryResult $qr A QueryResult returned from a call to DB::query()
   */
  function formatAsTabbed($qr) {
    $str = "LIST OF COMPETITORS BY AFFILIATION\n";
    $curLeaderAffil=-1;
    $numRows=$qr->numRows();
    if (0==$numRows)
      $str.="none registered\n";
    else {
      for ($i=0;$i<$numRows;$i++) {
	list($num,$lfname,$llname,$laffil,
	     $ffname,$flname,$faffil) = $qr->getRowAt($i);
	if ($curLeaderAffil!=$laffil)  {
	  $str.="$laffil\n";
	  $curLeaderAffil=$laffil;
	}
	$str.="$num\t$lfname\t$llname\t$laffil\t$ffname\t$flname\t$faffil\n";
      }
    }
    return $str;
  }

  /**
   * Return the list as a pre-formatted text list
   *
   * This format is designed for use by authors who wish to
   * simply drop the output directly into thier printed program with
   * little to no modifications.
   * @param QueryResult $qr A QueryResult returned from a call to DB::query()
   */
  function formatAsText($qr) {
    $str = "List of Competitors, by Affiliation, by Competitor Number\n";
    $curLeaderAffil=-1;
    $numRows=$qr->numRows();
    if (0==$numRows)
      $str.="none registered\n";
    else {
      $storedNum=-1;
      for ($i=0;$i<$numRows;$i++) {
	list($num,$lfname,$llname,$laffil,
	     $ffname,$flname,$faffil) = $qr->getRowAt($i);
	if ($curLeaderAffil!=$laffil) {
	  $str.="$laffil\n";
	  $curLeaderAffil=$laffil;
	}
	if ($faffil!=$laffil) $faffil=" ($faffil)"; else $faffil="";
	if ($storedNum!=$num) {
	  $storedNum=$num;
	  $str.="$num $lfname $llname & $ffname $flname$faffil\n";
	} else {
	  $str.="\t\t& $ffname $flname$faffil\n";
	}
      }
    }
    return $str;
  }
}

Code: Select all

<?php
  /**
   * Format lists of event registrations into end-user friendly formats.
   * This file is part of CompInaBox.
   * @copyright CompInaBox Copyright 2001-2005. Eric D. Nielsen, All rights reserverd.
   * @license http://opensource.org/licenses/gpl-license.php GNU Public License
   *
   * @author Eric D. Nielsen <nielsene@alum.mit.edu>
   * @package DataFormatting
   * @filesource
  */
  /**
   * Formats QueryResults for use in outside documentation.
   *
   * This class handles formatting list of registrations into the
   * various layouts needed for people assembling a printed program
   * offering choices such as tabbed delimited or pre-formatted text.
   * @package DataFormatting
   */
class EventListFormatter {
  function EventEntryListFormatter() {
  }

  /**
   * Return the list as tab delimited entries
   *
   * This format is designed for use by authors who wish to
   * do significant data maniupulation on the output.
   * @param QueryResult $qr A QueryResult returned from a call to DB::query()
   */
  function formatAsTabbed($qr,$nestedFormatter) {
    $str = "LIST OF EVENTS\n";
    $numRows=$qr->numRows();
    if (0==$numRows)
      $str.="No Events\n";
    else {
      for ($i=0;$i<$numRows;$i++) {
	list($eventID,$progNum,$eventName) = $qr->getRowAt($i);
	$str.="$progNum. $eventName\n";
	$registry = Registry::instance();
	$pb =& $registry->getResource("Export Phrase Book");
	
	$db =& $registry->getResource("Competition Database");
	$query = $pb->getQuery("Event Registrations",
			       array("eventid"=>$eventID));
	$subResult = $db->query($query);
	$str.= $nestedFormatter->formatAsTabbed($subResult);
	$str.="\n";
      }
    }
    return $str;
   
  }

  /**
   * Return the list as a pre-formatted text list
   *
   * This format is designed for use by authors who wish to
   * simply drop the output directly into thier printed program with
   * little to no modifications.
   * @param QueryResult $qr A QueryResult returned from a call to DB::query()
   */
  function formatAsText($qr,$nestedFormatter) {
    $str = "List of Registrations\n";
    $numRows=$qr->numRows();
    if (0==$numRows)
      $str.="No Events\n";
    else {
      for ($i=0;$i<$numRows;$i++) {
	list($eventID,$progNum,$eventName) = $qr->getRowAt($i);
	$str.="Event Number $progNum. $eventName\n";
	$registry = Registry::instance();
	$pb =& $registry->getResource("Export Phrase Book");
	
	$db =& $registry->getResource("Competition Database");
	$query = $pb->getQuery("Event Registrations",
			       array("eventid"=>$eventID));
	$subResult = $db->query($query);
	$str.= $nestedFormatter->formatAsText($subResult);
	$str.="\n";
      }
    }
    return $str;
  }
}
?>

Code: Select all

<?php
  /**
   * Format a single event registrations into end-user friendly formats.
   * This file is part of CompInaBox.
   * @copyright CompInaBox Copyright 2001-2005. Eric D. Nielsen, All rights reserverd.
   * @license http://opensource.org/licenses/gpl-license.php GNU Public License
   *
   * @author Eric D. Nielsen <nielsene@alum.mit.edu>
   * @package DataFormatting
   * @filesource
  */
  /**
   * Formats QueryResults for use in outside documentation.
   *
   * This class handles formatting list of registrations into the
   * various layouts needed for people assembling a printed program
   * offering choices such as tabbed delimited or pre-formatted text.
   * @package DataFormatting
   */
class EventEntryListFormatter {
  function EventEntryListFormatter() {
  }

  /**
   * Return the list as tab delimited entries
   *
   * This format is designed for use by authors who wish to
   * do significant data maniupulation on the output.
   * @param QueryResult $qr A QueryResult returned from a call to DB::query()
   */
  function formatAsTabbed($qr,$full=FALSE) {
    $str = "";
    $numRows=$qr->numRows();
    if (0==$numRows)
      $str.="No one entered.\n";
    else {
      for ($i=0;$i<$numRows;$i++) {
	list($coupleNum,$lfirst,$llast,$laffil,
	     $ffirst, $flast,$faffil) = $qr->getRowAt($i);
	if ($full)
	  $str.="$coupleNum\t$lfirst\t$llast\t$laffil\t$ffirst\t$flast\t$faffil\n";
	else
	  $str.="$coupleNum\n";
      }
    }
    return $str;
  }

  /**
   * Return the list as a pre-formatted text list
   *
   * This format is designed for use by authors who wish to
   * simply drop the output directly into thier printed program with
   * little to no modifications.
   * @param QueryResult $qr A QueryResult returned from a call to DB::query()
   */
  
  function formatAsText($qr,$full=FALSE) {
    $str = "";
    $numRows=$qr->numRows();
    if (0==$numRows)
      $str.="No one pre-registered.\n";
    else {
      $lastRow=$numRows-1;
      for ($i=0;$i<$numRows;$i++) {
	list($num,$lfirst,$llast,$laffil,
	     $ffirst,$flast,$faffil) = $qr->getRowAt($i);
	if ($full) {
	  $str.="$num $lfirst $llast ($laffil) & $ffirst $flast ($faffil)\n";
	} else {
	  if (($i!=0) && (($i%8)==0)) $str.="\n";
	  if (($i%8)!=0) $str.="  ";
	  $str.=$num;
	}
      }
      if (!$full) $str.="\n";
    }
    $str.="\n1st:______ 2nd:______ 3rd:______ 4th:______ 5th:______ 6th:______ 7th:______\n";
    return $str;
  }
  
}
?>
McGruff
DevNet Master
Posts: 2893
Joined: Thu Jan 30, 2003 8:26 pm
Location: Glasgow, Scotland

Post by McGruff »

Will reply soon - I'd like to see this through. Need some time to think but time is in short supply.
User avatar
nielsene
DevNet Resident
Posts: 1834
Joined: Fri Aug 16, 2002 8:57 am
Location: Watertown, MA

Post by nielsene »

No problem. I think I might have a solution, but I'm unsure how to proceed.

I have managed to write a common base class, but I'm not sure how to migrate to it properly and stay under the TDD harness. After staring at both of the TDD books and the refactoring book, I think I know what to do, but it still seems a little shaky.

I think the process is:

For each method to move to the new base class:

1st One concrete class at a time, I need to slowly add in the common control code and making their constuctors set the appropriate options. Running the tests, of course, at each atomic little change.

2nd. Once all the concrete classes match, copy the common method code into the base class. Re-run tests

3rd. Remove the common method for one class at a time, rerunning tests.

It looks like the common code is slightly more expressive than any of the old subclasses. However I shouldn't write any new tests for the base class as that's not functionality that's needed by the application.

I'm not sure if the revised version is "better". Once I finish the refactoring, I'll most the new version of the class hierarchy. I had to introduce a lot fo extra control code that might make it worse....
User avatar
nielsene
DevNet Resident
Posts: 1834
Joined: Fri Aug 16, 2002 8:57 am
Location: Watertown, MA

Post by nielsene »

Have managed to refactor the first two classes down to using a common base class for most things. There's still a minor amount of code duplication, but I think trying to abstract more away would just make it less clear. Going to continue on the next two classes tomorrow.

This has definitely been an experience demonstrating the power of TDD... Some of the changes were rather "radical" and it was very nice to know instantly if I messed up or not.
McGruff
DevNet Master
Posts: 2893
Joined: Thu Jan 30, 2003 8:26 pm
Location: Glasgow, Scotland

Post by McGruff »

Good to hear - I'm glad you're noticing some payback for all that effort.
User avatar
nielsene
DevNet Resident
Posts: 1834
Joined: Fri Aug 16, 2002 8:57 am
Location: Watertown, MA

Post by nielsene »

So the refactoring is complete.

Generall speaking each subclass had to only override one function, some of the more complex ones needed a second helper function. Overall, I think its "better" than what I started with, but I still wish I could further reduce some of the repetition. The _format() method in the base class handles most of the work and the four(five) list types and both formats are handled there. However the two types are not unified further up the tree. I'm trying to decide if i should fold the $type into the $context and/or other ways of unifying the two further up the call chain.

Here's the current state of things (stipping out most of the PHPDoc blocks to keep this shorter):

Base Class

Code: Select all

<?php
class ListFormatter {
  var $listTitle;
  var $lookback;
  var $emptyString;
  var $footer;

  function ListFormatter($title,$emptyString,
			 $lookback=array(), $footer="") {
    if (is_array($title)) {
      $this->listTitle=$title;
    } else {
      $this->listTitle["Tabbed"]=strtoupper($title);
      $this->listTitle["Text"]=$title;
    }
    $this->lookback=$lookback;
    if (is_array($emptyString)) {
      $this->emptyString=$emptyString;
    } else {
      $this->emptyString["Tabbed"]=$emptyString;
      $this->emptyString["Text"]=$emptyString;
    }
    if (is_array($footer)) {
      $this->footer=$footer;
    } else {
      $this->footer["Tabbed"]=$footer;
      $this->footer["Text"]=$footer;
    }
  }

  function _usesSectionLookback($type) {
    return isset($this->lookback["$type"]["Section"]);
  }

  function _usesRowLookback($type) {
    return isset($this->lookback["$type"]["Row"]);
  }

  /**
   * Outputs an array with a tab between each item and
   * a newline at the end
   * @param array $arr list of elements to output, tab seperated
   * @param array $context not used in this implementation
   * @return string a single row for the output
  */
  function _printTabbed($arr,$context) {
    $row="";
    foreach($arr as $item) {
      if ($row!="") $row.="\t";
      $row.=$item;
    }
    $row.="\n";
    return $row;
  }


  /**
   * Format the list, using callbacks to the concrete classes.
   *
   * @param QueryResult $qr A QueryResult returned from a call to DB::query()
   * @param string $type Used to identify the desired format
   * @param array $baseContext Contest information needed by the outputter
   * @return string a formatted string
   * @todo Look into splitting type out to a strategy?
   */
  function _format($qr,$type,$baseContext=array()) {
    $str = $this->listTitle[$type];
    if ($this->_usesSectionLookback($type)) {
      $sectionVal= $this->lookback[$type]["Section"]["Initial"];
    }
    if ($this->_usesRowLookback($type)) {
      $rowVal= $this->lookback[$type]["Row"]["Initial"];
    }
    $numRows=$qr->numRows();
    if (0==$numRows)
      $str.=$this->emptyString[$type];
    else {
      for ($i=0;$i<$numRows;$i++) {
	$vals = $qr->getRowAt($i);
	$context = $baseContext;
	$context["Index"]=$i;
	$context["NumRows"]=$numRows;
	if ($this->_usesSectionLookback($type) &&
	    $sectionVal!=$vals[$this->lookback[$type]["Section"]["Field"]]) {
	  $str.=$this->_processSectionLookback($vals,$type,$sectionVal,$context);
	}
        if ($this->_usesSectionLookback($type)) $context["Section"]=$sectionVal;
        if ($this->_usesRowLookback($type)) $context["Row"]=&$rowVal;
        $funcName="_print$type";
        $str.=$this->$funcName($vals,$context);
      }
    }
    $str .= $this->footer[$type];
    
    return $str;
  }

  /**
   * Return the list as a tabbed text list
   *
   * @param QueryResult $qr A QueryResult returned from a call to DB::query()
   */
  function formatAsTabbed($qr,$baseContext=array()) {
    return $this->_format($qr,"Tabbed",$baseContext);
  }



  /**
   * Return the list as a pre-formatted text list
   *
   * @param QueryResult $qr A QueryResult returned from a call to DB::query()
   */
  function formatAsText($qr,$baseContext=array()) {
    return $this->_format($qr,"Text",$baseContext);
  }

  /**
   * Abstract function, must be overrided for text output
   */
  function _printText($arr,$context) {
    die("Abstract function called");
  }
}
?>
And here's the four sub-classes:

Code: Select all

<?php
class CompetitorListFormatter extends ListFormatter{
  function CompetitorListFormatter() {
    parent::ListFormatter(array("Tabbed"=>"LIST OF COMPETITORS\n",
				"Text"=>"List of Competitors, by Competitor Number\n"),
			  array("Tabbed"=>"none registered\n",
				"Text"=>"none registered\n"),
			  array("Tabbed"=>array(),
				"Text"=>array("Row"=>array("Initial"=>-1,
							   "Field"=>0))),
			  array("Tabbed"=>"","Text"=>""));
  }
  
  /**
   * Implement the Textual formatter
   *
   * Generates the pre formatted text list, collapse identical leaders
   * @param array $arr the current row from the DB
   * @param array $context Tracks the last leader seen for collapsing checks
   * @return string the next row fo the list
   */
  function _printText($arr,$context) {
    list($num,$lfname,$llname,$laffil,
	 $ffname,$flname,$faffil) = $arr;
    $row="";
    if ($this->_usesRowLookback("Text") && $context["Row"]==$num) {
      $row.="\t\t& $ffname $flname ($faffil)\n";
    } else {
      $context["Row"]=$num;
      $row.="$num $lfname $llname ($laffil) & $ffname $flname ($faffil)\n";
    }
    return $row;
  }


}
?>
and

Code: Select all

<?php
class EventListFormatter extends ListFormatter{
  function EventListFormatter() {
    parent::ListFormatter(array("Tabbed"=>"LIST OF EVENTS\n",
				"Text"=>"List of Registrations\n"),
			  array("Tabbed"=>"No Events\n",
				"Text"=>"No Events\n"),
			  array("Tabbed"=>array(),
				"Text"=>array()),
			  array("Tabbed"=>"","Text"=>""));
  }

  function _retrieveEventsEntries($eventID) {
    $registry = Registry::instance();
    $pb =& $registry->getResource("Export Phrase Book");
    
    $db =& $registry->getResource("Competition Database");
    $query = $pb->getQuery("Event Entries By Competitor Number",
			   array("eventid"=>$eventID));
    return $db->query($query);
  }

  function _printTabbed($vals,$context){
    list($eventID,$progNum,$eventName) = $vals;
    $subResult = $this->_retrieveEventsEntries($eventID);

    $str="$progNum. $eventName\n";
    $str.= $context["Formatter"]->formatAsTabbed($subResult,$context);
    $str.="\n";
    return $str;
  }

  function _printText($vals,$context){
    list($eventID,$progNum,$eventName) = $vals;
    $subResult =$this->_retrieveEventsEntries($eventID);

    $str="Event Number $progNum. $eventName\n";
    $str.= $context["Formatter"]->formatAsText($subResult,$context);
    $str.="\n";
    return $str;
  }
}
?>
and

Code: Select all

<?php
class EventEntryListFormatter extends ListFormatter{
  function EventEntryListFormatter() {
    parent::ListFormatter(array("Tabbed"=>"",
				"Text"=>""),
			  array("Tabbed"=>"No one entered.\n",
				"Text"=>"No one pre-registered.\n"),
			  array("Tabbed"=>array(),
				"Text"=>array()),
			  array("Tabbed"=>"","Text"=>"\n1st:______ 2nd:______ 3rd:______ 4th:______ 5th:______ 6th:______ 7th:______\n"));
  }


  function _printTabbed($arr,$context) {
    list($coupleNum,$lfirst,$llast,$laffil,
	 $ffirst, $flast,$faffil) = $arr;
    $row = "";
    if (isset($context["Full"]) && $context["Full"])
      $row.="$coupleNum\t$lfirst\t$llast\t$laffil\t$ffirst\t$flast\t$faffil\n";
    else
      $row.="$coupleNum\n";
    return $row;
  }

  function _printText($arr,$context) {
    list($coupleNum,$lfirst,$llast,$laffil,
	 $ffirst, $flast,$faffil) = $arr;
    $row = "";
    if (isset($context["Full"]) && $context["Full"])
      $row.="$coupleNum $lfirst $llast ($laffil) & $ffirst $flast ($faffil)\n";
    else {
      $i = $context["Index"];
      if (($i!=0) && (($i%8)==0)) $row.="\n";
      if (($i%8)!=0) $row.="  ";
      $row.=$coupleNum;
      if ($i==($context["NumRows"]-1)) $row.="\n";
    }
    return $row;
  }

}
?>
and lastly

Code: Select all

<?php
class CompetitorAffilListFormatter extends ListFormatter {
  function CompetitorAffilListFormatter() {
    parent::ListFormatter(array("Tabbed"=>"LIST OF COMPETITORS BY AFFILIATION\n",
				"Text"=>"List of Competitors, by Affiliation, by Competitor Number\n"),
			  array("Tabbed"=>"none registered\n",
				"Text"=>"none registered\n"),
			  array("Tabbed"=>array("Section"=>array("Initial"=>-1,
								 "Field"=>3)),
				"Text"=>array("Row"=>array("Initial"=>-1,
							   "Field"=>0),
					      "Section"=>array("Initial"=>-1,
							       "Field"=>3))),
			  array("Tabbed"=>"","Text"=>""));
  }

  /** Outputs a new section header, updating the current section */
  function _processSectionLookback($vals,$type,&$sectionVal,$context=array()) {
    $sectionVal = $vals[$this->lookback[$type]["Section"]["Field"]];
    return "$sectionVal\n";
  }
  

  function _printText($arr,$context) {
    list($num,$lfname,$llname,$laffil,
	 $ffname,$flname,$faffil) = $arr;
    $row="";
    if ($context["Section"]!=$faffil) $faffil=" ($faffil)"; else $faffil="";
    if ($context["Row"]==$num) {
      $row.="\t\t& $ffname $flname$faffil\n";
    } else {
      $context["Row"]=$num;
      $row.="$num $lfname $llname & $ffname $flname$faffil\n";
    }
    return $row;
  }
}
Post Reply