Page 1 of 1

Generall OOP feedback wanted.

Posted: Sun Apr 26, 2009 12:05 pm
by JasonDFR
I'm teaching myself object oriented php and have moved just beyond the basics (I think).

I'd like some general feedback about the code below. Where have I gone wrong? Etc. I'm interested in how I am, or should be, updating the subscriber information to the mysql database I'm using. I've got a database table class that I send the attributes of the "subscriber" to everytime a change in the subscriber's attributes is made. I'm not sure if I've got all the code in the right places. I know it is around 100 lines, but if you could take a minute and provide some general feedback, that would be great. I've been kinda putting things together based on a mixture of what I've read and what "I think" is right. And I'm well aware that what I think could very well be very wrong. So please be as critical as you want. I need some feedback.

Thanks a lot.

Code: Select all

 
class Subscriber extends User {
 
    /**
     * Is active?
     *
     * @access protected
     * @var bool
     */
    protected $_active = false;
 
    /**
     * Is registered?
     *
     * @access protected
     * @var bool
     */
    protected $_registered = false;
 
    /**
     * Is confirmed?
     *
     * @access protected
     * @var bool
     */
    protected $_confirmed = false;
 
    /**
     * Database Table to interact with
     *
     * @access protected
     * @var Zend_Db_Table object
     */
    protected $_db;
 
    public function __construct($emailAddress, $attributes = null) {
        $this->_db = new NewsletterTb;
 
        $validator = new Zend_Validate_EmailAddress;
        if (!$validator->isValid($emailAddress)) {
            throw new Exception('A valid email address is required to be passed to Subscriber');
        }
        unset($validator);
 
        $validator = new Zend_Validate_Db_RecordExists('newsletter', 'emailAddress');
        //$subscriber = $this->_db->getSubscriber($emailAddress);
        if (!$validator->isValid($emailAddress)) { // Email address does not exist in db
            $this->_emailAddress = $emailAddress;
            $this->setRand();
        } else { // Email address does exist in db
            $this->_emailAddress = $emailAddress;
            $this->buildUser();
        }
 
    }
 
    protected function buildUser() {
        $subscriber = $this->_db->getSubscriber($this->_emailAddress);
        $this->_id = $subscriber['id'];
        $this->_emailAddress = $subscriber['emailAddress'];
        $this->_firstName = $subscriber['firstName'];
        $this->_lastName = $subscriber['lastName'];
        $this->_active = $subscriber['active'];
        $this->_confirmed = $subscriber['confirmed'];
        $this->_rand = $subscriber['rand'];
        $this->_registered = true;
    }
 
    public function isRegistered() {
        return $this->_registered;
    }
 
    public function isActive() {
        return $this->_active;
    }
 
    public function isConfirmed() {
        return $this->_confirmed;
    }
 
    public function register($insert = array()) {
 
        if (!$this->isRegistered()) {
            $insert['rand'] = $this->_rand;
            $result = $this->_db->newEntry($this->_emailAddress, $insert);
            return $result;
        } else {
            throw new Exception('This subscriber is already registered');
        }
    }
 
    public function update() {
 
        $update = array();
        $update['id'] = $this->_id;
        $update['emailAddress'] = $this->_emailAddress;
        $update['firstName'] = $this->_firstName;
        $update['lastName'] = $this->_lastName;
        $update['rand'] = $this->getRand();
        $update['active'] = $this->_active;
        $update['confirmed'] = $this->_confirmed;
 
        return $this->_db->updateEntry($update);
 
    }
 
    public function sendConfirmEmail() {
 
        $rand = $this->_rand;
 
        $subject = '';
        $config = Zend_Registry::getInstance()->Zend_Config;
 
        $body = '<h2>Click the link below to confirm your subscription</h2>' . "\r\n";
        $body .= '<p><a href="http://' . $config->url . '/newsletter/confirm/email/' . $this->_emailAddress . '/rand/' . $rand . '">Click here to confirm your subscription.</a></p>';
 
        $mail = new Zend_Mail();
        $mail->setBodyHTML($body);
        $mail->setFrom($config->reply->email, $config->reply->name);
        $mail->addTo($this->_emailAddress);
        $mail->setSubject($subject);
 
        try {
            return $mail->send();
        } catch (Exception $e) {
            $logger = $registry = Zend_Registry::getInstance()->Zend_Log;
            $logger->err($e->getMessage(). ' ' . $this->_emailAddress . ' : Error sending subscribe confirm email');
            return false;
        }
    }
 
    public function confirmSubscription($rand) {
 
        if ($this->_rand == $rand) {
            $this->confirmed = true;
            $this->update();
            return true;
        }
 
        return false;
    }
 
    public function unsubscribe() {
        $this->_active = false;
        $this->update();
    }
}
 

Re: Generall OOP feedback wanted.

Posted: Sun Apr 26, 2009 3:33 pm
by Benjamin
:arrow: Moved to Coding Critique

Re: Generall OOP feedback wanted.

Posted: Sat May 02, 2009 1:10 am
by Cirdan
My opinion is to not have "sendConfirmEmail" inside the Subscriber class because sendConfirmEmail doesn't really have anything to do with Subscriber, it has to do with registering a new account. The only thing related to Subscriber is the $_rand variable.

I would have a function to set the confirmation code, then another function to confirm the subscription like you have.