CODEWITHSUNDEEP
phpoop
Dima Dz
Dima Dz

Reputation: 538

Constructor injection for two mutually dependent classes

A user fills in the form and submits it. Based on the input, an object Organization is hydrated. I want to separate communication with database from the actual object.

I thought of creating an OrganizationMapper that holds the methods for database communication (save, delete...). The organization class would get the OrganizationMapper through the constructor.

With these class definitions, however, I can't instantiate the classes because of their mutual dependence.

How else could I separate the database communication from Organization and put it into OrganizationMapper?

class Organization
{
    protected $id;
    protected $name;
    ... other properties ...
    public function __construct(OrganizationMapper $mapper)
    {
        $this->mapper = $mapper;
    }
    public function getId() {...}
    public function setId($id) {...}
    ... other methods ...
    public function saveToDb()
    {
        $this->mapper->save($this);
    }

The OrganizationMapper is

class OrganizationMapper
{
     public function __construct(Organization $organization)
     {
         $this->organization = $organization
     }

     ... other methods

     public function save($organization)
     {... the code to use the methods of Organization class to save the data to the database...}
}

Upvotes: 1

Views: 695

Answers (5)

Sebastian Malaca
Sebastian Malaca

Reputation: 111

I don't know why Mapper should do any opperations on DB? Mapper sounds like converting Entity (Organization) into something that can be an input for DB operation ie. Query Object. You should rename your class into DAO or Repository. It would be better name.

IMHO, the best idea would be to have:

  1. Organization as an object that holds domain logic
  2. OrganizationMapper should convert your domain object into some kind of query object
  3. OrganizationDao should take Organization as an input param and use OrganizationMapper to convert it and do operation on DB.

BTW, why you are not using some kind of an ORM like Doctrine for example? It would make your life easier :)

Upvotes: 1

Félix Adriyel Gagnon-Grenier
Félix Adriyel Gagnon-Grenier

Reputation: 8885

Disclaimer: I name your Organization type as OrganizationEntity in this post.

Pretty simply, it's the other way around.

The OrganisationMapper gets an OrganisationEntity object and persists it to wherever you want to, by means you can choose.

For your problem:

move the saveToDb() method from your OrganisationEntity to the OrganisationMapper and pass it an object to be saved.

Upvotes: 1

Matthias
Matthias

Reputation: 2775

To find a better way of seperating these two concerns, think about the purposes of your two objects:

  • an Organization is there to give you access to all informations of an organization
  • your OrganizationMapper is there to save a Organization object to database.

When you think about it like this, then there's a couple of questions, that rise up:

  • Why does your Organization need a saveToDb() method? It's not it's job to save it?
  • An instance of OrganizationMapper should be able to save any Organization in the database, so why do you pass it in twice? (once in the constructor, and once in the save($organization) method). In that case - what happens, if you pass a different organization to the constructor than to the save method?
  • In your current example, how would you load an Organization from Database?

As alternative, I would suggest to remove saveToDb() from Organization entirely, as it's not the job of the org to save itself to database. Additionally, I would remove the current Constructor from OrganizationMapper. In it's current design, there's little reason to pass the Organization to the constructor. Also, I would rename the OrganizationMapper to OrganizationRepository or OrganizationService. The primary purpose of that class is not to map SQL to Objects but to retrieve/save Organizations from/to DB. (Also, in OOP, classes should only follow the single responsibility pattern, so maybe the part mapping between SQL and Objects should happen in specializied class)

As a side note: generally, it's not a great idea, to give many ways to do exactly the same thing (e.g. saving an organization). This will probably just cause inconsistencies over time (consider that you will be adding some validation logic in the future, but might forget to also add it in the second place).

I hope this helps you :)

Upvotes: 2

Luka Govedič
Luka Govedič

Reputation: 524

You can't do that in php. Imagine if it would be posibble. Then instance of Organization would have a property OrganizationMapper, which would have a property Organization. So, property of a property of an instance of the class would be the instance itself! It is only possible in languages with pointers like c++. So, I see only 2 solutions here:

  • Put the classes together
  • Have a single link (maybe have 1 class that calls another while second doesn't call first.)

Upvotes: 0

helmbert
helmbert

Reputation: 38004

And that's why circular dependencies are usually considered a bad thing.

Kidding aside, it seems to me that you do not actually need the constructor dependency in the OrganizationMapper class. From the looks of it, you're passing the Organization instance that you want to persist as a parameter into the mapper's save() method anyway and shouldn't need the instance attribute $this->organization in that class at all.

In general, I'd try to keep the OrganizationMapper stateless. Try to avoid storing an Organization instance as an instance attribute (especially if you actually use that same mapper instance for persisting multiple Organizations). Just do as you already did with the save() method and pass the Organization object as a method parameter.

Also, I would not associate the Organization class with the mapper. One could argue that this violates the Single Responsibility Principle as it's not the class' responsibility to persist itself. You could move this logic to the calling code and have the Organization class not know about the mapper at all (which is nice, because you completely eliminate the circular dependency between the two classes):

class Organization
{
    protected $id;
    protected $name;
    // <other properties here>

    // <getters and setters here>
}

class OrganizationMapper
{
    public function save(Organization $organization)
    {
        // save $organization to DB, somehow
    }
}

$organization = new Organization();
$organization->setName('Foobar International Inc.');

$mapper = new OrganizationMapper();
$mapper->save($organization);

Upvotes: 4

Related Questions