2
\$\begingroup\$

This example is simplified down to the bare minimum. No DatabaseAdapter IdentityMap or anything. No fetch() method. Just what is needed to demonstrate this.

If all your data mappers have a method:

/**
 * @return EntityType
 */
public function assign(EntityType $entity, Array $row);

You are basically creating a new blank entity each time you want to assign data to it.

I have began thinking what if all the data mappers get an entity of the related type injected into its constructor:

UserMapper::__construct(User $user)

Now the

UserMapper::assign(User $user, Array $row)

method could be changed to

UserMapper::assign(Array $row);

Since the mapper has an instance of a User object which it got when the mapper was constructed and it just assigns the row data to that.

There is a slight problem though because if I do something like the following:

$user1 = $userMapper->assign( array('username' => 'user1') );
$user2 = $userMapper->assign( array('username' => 'user2') );

// Now $user1 has the same username as $user2.
$user2->setUsername('user2_something');

Any changes I make to either user instance reflects in the other instance.

This can be solved by cloning the single user instance in the mapper when returning it in the assign method:

public function assign(Array $row) {
    $this->user->setUsername( $row['username'] );
    return clone $this->user;
}

UPDATE: Or Maybe Better...

// Now the user object in the mapper never changes, just gets cloned.
public function assign(Array $row) {

    $user = clone $this->user;
    $user->setUsername( $row['username'] );

    return $user;
}

And now the user instance you get back from the UserMapper::assign(Array $row) method does not point to the user instance inside the mapper.

I have never had to use the clone feature before so I am not too sure about it.

Is this implementation okay? Could it cause any problems?

\$\endgroup\$
2
  • \$\begingroup\$ Which library do you use? There are at least several PHP libraries that claim DataMapper implementation and all of them are different in details. \$\endgroup\$ Nov 8, 2013 at 1:23
  • \$\begingroup\$ I don't use any library for this. I have seen a few implementations of data DataMapper but never really liked any of them. I came up with this idea yesterday and wanted to see if anyone can find any pitfalls in it. It is nicer and easier to just do $user = $userMapper->assign($userDataArray); instead of first creating a User object and passing it in also. The mapper is for transferring of data between domain objects and that is what it does. I have seen examples where the mapper creates the actual entity. I don't think it is its job to do that. In my example it keeps cloning 1 entity. \$\endgroup\$
    – ibanore
    Nov 8, 2013 at 9:43

1 Answer 1

1
\$\begingroup\$

I can't speak to cloning specifically, as I've never used it before, but here are a few basic things to think about.

Is the User class used in any other methods of your UserMapper?

If it is then injecting it in the constructor, or through a setter is ideal as it no longer depends on a user object being passed to it in each method.

This is basic OOP.

Are you perhaps trying to do too much for the scope of this class?

If we follow the Single Responsibility Principle, as we should, then our classes should be responsible for just one thing. According to WikiPedia:

A Data Mapper is a Data Access Layer that performs bidirectional transfer of data between a persistent data store (often a relational database) and an in memory data representation (the domain layer).

To me, this means that a Data Mapper is essentially a database read/write object. What you have here seems to be a Factory object that builds individual User objects. If what you are trying to accomplish is indeed a Factory and not a Data Mapper, then you should call it what it is to avoid confusion. But for a Data Mapper the assign() method seems to be out of scope. You might consider refactoring your code, perhaps create a new class if necessary. If this is the only violation, then perhaps a better way of doing this would be to simply create the new object out of the class and remove the assign() method.

Is your User class perhaps doing too much?

It seems odd seeing a user object that allows you to change its name on the fly like this. Most user objects, that I'm familiar with, perform these kinds of operations in instantiation. Allowing this kind of change is invisible to the rest of your code. In other words, between one method and the next, the object can change and your code wont know the difference. So it is possible to encounter "state" errors, such as information from a previous user not being overwritten by the new user.

Sorry I couldn't be more precise, but with so little to work with its hard to determine what you are trying to accomplish. Hope this helps.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.