HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Using ZF2 Event manager to save a model

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
savezf2usingmanagermodelevent

Problem

I have been reading about ZF2 EventManager for some time now and I wonder what you'd think about this approach.

class Model_Member extends App_Model_Model
{
    protected $_data = array(
        'ID' => null,
        'UserName' => null,
        'Password' => null,
        'EmailAddress' => null,
        'CreatedOn' => null,
        'Status' => 0,
        'PasswordResetHash' => null,
        'Role' => 'member'
    );
    const EVENT_SAVE = 'save';
    const EVENT_POSTSAVE = 'post.save';
    public function init()
    {
        $this->events()->attach(self::EVENT_SAVE, array('Service_Member', 'save'));
    }
    public function save()
    {
        $id = $this->events()->trigger(self::EVENT_SAVE, $this)->last();
        $this->ID = $id;
        $this->events()->trigger(self::EVENT_POSTSAVE, $this);
    }
}


And of course there is Service_Member

```
public function save($event)
{
// Get the member from the event
$member = $event->getTarget();
// If there is no id, save, otherwise update
if (!$member->ID) {
$id = $this->doSave($member);
$event->setParam('id', $id);
return $id;
} else {
return $this->doUpdate($member);
}
}
public function doSave(Model_Member $member)
{
// Check if username is taken
if ($this->checkIfExists('UserName', $member->UserName)) {
throw new App_Exception_UserNameExists('This username is already taken.');
}

// Check if email address already registered
if ($this->checkIfExists('EmailAddress', $member->EmailAddress)) {
throw new App_Exception_EmailAddressExists('This email is already registered.');
}

// Set default values
$member->CreatedOn = date('Y-m-d H:i:s');

// Encrypt password
$member->Password = sha1($member->Password);

// Get the data and save. Return the member with new ID
$data = $member->_toArray();
unset($data['ID']);
$this->getWriteDb()->insert('members', $data);
$id = $this->getWriteDb()->

Solution

I haven't used PHP recently, just a few, mostly minor notes:

-

// Encrypt password
$member->Password = sha1($member->Password);


You might want to use a salt here to make your hashing stronger.

-

$this->getWriteDb()->insert('members', $data);
$id = $this->getWriteDb()->lastInsertId();


If that's possible, insert could return the ID. It would eliminate temporal coupling (users can't call the methods in the wrong order).

(Clean Code by Robert C. Martin, G31: Hidden Temporal Couplings, p302)

-

'Status' => 0,


0 is a magic number here. What could it mean? A named constants would express the intent.

-
These comments are rather redundant, the code is clear and says the same:

// Check if username is taken
if ($this->checkIfExists('UserName', $member->UserName)) {
    throw new App_Exception_UserNameExists('This username is already taken.');
}

// Check if email address already registered
if ($this->checkIfExists('EmailAddress', $member->EmailAddress)) {
    throw new App_Exception_EmailAddressExists('This email is already registered.');
}


...

// Get the member from the event
$member = $event->getTarget();


(Clean Code by Robert C. Martin, Redundant Comments, p60)

-
Btw, are you sure that you need two different type of exceptions here? Wouldn't be the same exception type with different messages enough?

-
Use private instead of protected if possible:

protected $_data = array(


Should I always use the private access modifier for class fields?; Item 13 of Effective Java 2nd Edition: Minimize the accessibility of classes and members.

-

class Model_Member extends App_Model_Model


The class name is weird a little bit. Model of a model? App also too generic, something more descriptive would be better.

-

$member->CreatedOn = date('Y-m-d H:i:s');


Wouldn't be better using DateTime objects here?

Code Snippets

// Encrypt password
$member->Password = sha1($member->Password);
$this->getWriteDb()->insert('members', $data);
$id = $this->getWriteDb()->lastInsertId();
'Status' => 0,
// Check if username is taken
if ($this->checkIfExists('UserName', $member->UserName)) {
    throw new App_Exception_UserNameExists('This username is already taken.');
}

// Check if email address already registered
if ($this->checkIfExists('EmailAddress', $member->EmailAddress)) {
    throw new App_Exception_EmailAddressExists('This email is already registered.');
}
// Get the member from the event
$member = $event->getTarget();

Context

StackExchange Code Review Q#7132, answer score: 3

Revisions (0)

No revisions yet.