patternphpMinor
Using ZF2 Event manager to save a model
Viewed 0 times
savezf2usingmanagermodelevent
Problem
I have been reading about
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()->
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:
-
You might want to use a salt here to make your hashing stronger.
-
If that's possible,
(Clean Code by Robert C. Martin, G31: Hidden Temporal Couplings, p302)
-
-
These comments are rather redundant, the code is clear and says the same:
...
(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
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.
-
The class name is weird a little bit. Model of a model?
-
Wouldn't be better using DateTime objects here?
-
// 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_ModelThe 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.