Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

What's the OO way of refactoring these two very similar classes in PHP?

I have a class like the following:

class DreamsImagesStore
{
  public $table = 'dreams_images';

  public function insertNewDreamImage($dream_id, $pid)
  {
    try {
      $values = array($dream_id, $pid);
      $sth = $this->dbh->prepare("INSERT INTO {$this->table}
                                (dream_id, pid)
                                VALUES (?, ?)");
      if($sth->execute($values)) {
        return true;
      }
    } catch(PDOException $e) {
      $this->errorLogger($e);
    }
  }
...
}

I'm going to be implementing a new class called InterestsImagesStore wherein the only differences between these classes will be the value of $table, $dream_id will be $interest_id, and dream_id in SQL will be interest_id.

I know there's a better way to do this, and I'll be implementing similar classes in the future which have such small differences.

What's the best object-oriented way to refactor my code to avoid duplication and increase maintainability?

like image 523
Josh Smith Avatar asked Oct 04 '10 21:10

Josh Smith


2 Answers

Create an ImagesStore base class:

class ImagesStore
{
  // See comments about accessors below.
  private $table;
  private $id_column;

  public function insertImage($id, $pid) {
    try {
      $values = array($id, $pid);
      $table = $this->getTable();
      $id_column = $this->getIdColumn();
      $sth = $this->dbh->prepare("INSERT INTO {$table} ($id_column, pid) VALUES (?, ?)");
      if ($sth->execute($values)) {
        return true;
      }
    }
    catch (PDOException $e) {
      $this->errorLogger($e);
    }
  }

  protected function __construct($table, $id_column) {
    $this->table = $table;
    $this->id_column = $id_column;
  }

  // These accessors are only required if derived classes need access
  // to $table and $id_column. Declaring the fields "private" and providing
  // "protected" getters like this prevents the derived classes from
  // modifying these values which might be a desirable property of these
  // fields.
  protected function getTable() {return $this->table;}
  protected function getIdColumn() {return $this->id_column;}

  // More implementation here...
  // Initialize $dbh to something etc.
  // Provide "errorLogger" method etc.
}

And create DreamsImagesStore and InterestsImagesStore specializations:

class DreamsImagesStore extends ImagesStore {
  public function __construct() {
    parent::__construct('dreams_images', 'dream_id');
  }
}

class InterestsImagesStore extends ImagesStore {
  public function __construct() {
    parent::__construct('interests_images', 'interest_id');
  }
}

The original method insertNewDreamImage can be renamed to insertImage as it is really more general than the original name suggests.

Note that ImagesStore can also be declared abstract if you want to prevent direct instantiation of it.

An alternative approach that can be adopted is to not bother deriving classes from ImagesStore at all and just instantiate it directly by making the __construct method public and calling it as follows:

$dreamsImagesStore = new ImagesStore("dreams_images", "dream_id");

Another approach might also be to implement a static factory method in ImagesStore.

like image 183
Richard Cook Avatar answered Nov 03 '22 01:11

Richard Cook


using the ImagesStore class created by Richard cook, this could also happen:

function FactoryLoadImageStore($imageStoreType)
{
    switch($imageStoreType)
    {
        case "Interests":
            return new ImageStore('interests_images', 'interest_id');
        case "Dreams":
            return new ImageStore('dreams_images', 'dreams_id');
        default:
            throw new Exception("ImageStore type $imageStoreType not found")
;    }

}

or you could even get fancier and do something like

function FactoryLoadImageStore($imageStoreType)
{
    $tableName = $imageStoreType . 's_images';
    $idColumnName = $imageStoreType . 's_id';
    $tableExists = false;
    $sql= "Show tables like '$tableName' ";
    foreach ($this->dbh->query($sql) as $row)
    {
        if ($row['tableName'] == $tableName)
        {
            $tableExists = true;
            break;
        }
    }
    if( !$tableExists)
    {
        throw new Exception ("No matching table exists for the requested image store $imageStoreType");
    }

    return new ImageStore( $tableName, $idColumnName);
}

call as follows

$dreamImageStore = ImageStore::FactoryLoadImageStore('dream');
like image 24
Zak Avatar answered Nov 03 '22 00:11

Zak