Generall OOP feedback wanted.

Coding Critique is the place to post source code for peer review by other members of DevNetwork. Any kind of code can be posted. Code posted does not have to be limited to PHP. All members are invited to contribute constructive criticism with the goal of improving the code. Posted code should include some background information about it and what areas you specifically would like help with.

Popular code excerpts may be moved to "Code Snippets" by the moderators.

Moderator: General Moderators

Post Reply
JasonDFR
Forum Commoner
Posts: 40
Joined: Wed Jan 07, 2009 3:51 am

Generall OOP feedback wanted.

Post 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();
    }
}
 
User avatar
Benjamin
Site Administrator
Posts: 6935
Joined: Sun May 19, 2002 10:24 pm

Re: Generall OOP feedback wanted.

Post by Benjamin »

:arrow: Moved to Coding Critique
Cirdan
Forum Contributor
Posts: 144
Joined: Sat Nov 01, 2008 3:20 pm

Re: Generall OOP feedback wanted.

Post 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.
Post Reply