How to Properly Enforce Correct Usage of a Class Method?

Question

Our current ORM solution uses Data Mappers to represent tables / views in the database which then return a Collection object that can be used to iterate through the retrieved records as Model objects. Between the Data Mapper and Model layers is a Repository layer that handles domain requests to the data mappers and returns the corresponding collections or domain objects.

We are currently looking at refactoring the responsibilities of the Repository and Data Mapper layers so that all application requests to the Data Mapper layer are routed through the Repository and the Data Mapper returns the retrieved data rows to the Repository which then returns the necessary collection to the requesting object.

What I'm wondering is whether it is valid / good practice to pass the entire Repository object into the corresponding Data Mapper so that we can enforce access to the Data Mappers only through the Repository layer.

As an example this is basically how it works now:

class DataMapper {

 public function findAll(Criteria $criteria) 
 {
  $select = $criteria->getSelect();

  // Build specific select statement

  $rows = $this->_fetchAll($select);

  return new Collection(array('data' => $rows, 'mapper' => get_class($this)));
 }
}

I thinking of doing something like this:

class Repository {

 public function findAllByName(Model $model)
 {
  $this->_criteria->addCondition('name LIKE ?', $model->name);

  $rows = $this->_mapper->findAll($this);

  return new Collection(array('data' => $rows, 'repository' => get_class($this)));
 }

} 

class DataMapper {

 public function findAll(Repository $repository) 
 {
  $select = $repository->getCriteria()->getSelect();

  // Build specific select statement

  $rows = $this->_fetchAll($select);

  return $rows;
 }
}

And then in this version, the Collection object would issue a call to the Repository which could first search through its cached objects and then only issue a call to the database to load the record if its needed.

Answer 1

I would perhaps do this a little bit differently. I would add a setRepository(Repository $repos) method as well as a getRepository() method. Then, in your findAll method, call getRepository(). If setRepository() has not been called yet, then getRepository can return a default repository instance.

I would also perhaps create an interface for the Repository class so that different implementations of Repository can be used within the DataMapper class.

So the get method could look something like

public function getRepository()
{
    if (!$this->_repository) {
        $this->_repository = new Repository();
    }

    return $this->_repository;
}

and the set method could look something like

public function setRepository(RepositoryInterface $repos)
{
    $this->_repository = $repos;
}

Answer 2

Chris has a valid suggestion.

It partially depends on the context of the code, but dependency injecting the repository into the DataMapper instances you create, i.e:

$repo = new Repository();

$mapper = new DataMapper($repo);

Will spare you from subsequently having to pass that $repo around whenever you want to use findAll(). IE:

$mapper->findAll();
$mapper->findAllByName();

I find when parameters become a ubiquitous part of every function call I'm making, it makes sense to consider turning them into instance variables (especially when they're identical every time).

If your repository varies from between context/instances then the injection makes more sense. If you find that you're always creating one repo instance and would like to recycle it, a singleton might be appropriate.

The nice thing about Dependency Injection is it does clarify this dependency idea (ironic!). If you want to enforce it, you can do something like throw an exception if the $repo object is null, or not a Repository instance, in your __construct() method.