views:

87

answers:

2

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?

+10  A: 

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.

Richard Cook
Thanks for this. In `insertImage()`, don't you mean `$this->table` rather than `$table`? If not, why?
Josh Smith
+1 , but is it really even necessary to subclass with the hardcoded values? What about creating a factory function that takes the store type as a param, and returns an appropriate instance of the images store?
Zak
@Josh Smith: Either approach works. This implementation illustrates accessing these fields via the getter methods but the fields can equally well be accessed directly as `$this->table` and `$this->id_column` from within base class methods. Methods in the derived classes would have to use `getTable` and `getIdColumn` instead since I've declared the fields `private`. For code in base class methods either is correct and it comes down to taste.
Richard Cook
@Zak: +1: the factory pattern is a perfectly good solution also. The choice between class hierarchy and factory depends on many other factors that I haven't been mentioned here.
Richard Cook
Oh, right. I somehow missed the `getTable()` and `getIdColumn` methods.
Josh Smith
@Zak I'd love to see an answer that shows Factory at work and discusses pros/cons.
Josh Smith
+1 for the implementation using `__construct()` as `public`. Avoids file-tree mess. Any downsides to that approach (aside from figuring out how to document all the possible implementations)?
Josh Smith
To answer my own question/comment above, since this is a refactoring question, creating the subclasses avoids requiring changes to the rest of the existing code base by leaving in the existing class. The factory method can also be added, and the specific class usage of the DreamsImagesStore refactored out to use the Factory Method later. Creating the class hierarchy first for the DreamsImagesStore is required for a proper refactoring.
Zak
+1  A: 

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');
Zak
can't remember if the table name comes back as tableName in the row.. should be checked...
Zak