Critique my email class

[RyanMinor]

I am in the process of building my own MVC framework (just to learn the concepts) and I decided to throw some libraries and helpers in the mix to make things more convenient. Below is my email helper in which I want to be able to use an array as the "to" part of the mail() function. I was wondering what everyone thought of my class and if I can improve upon it. Thanks!

 

<?php 
/**
*	To use this email class in it's most basic form:
* 
* 		$to must be an array even if you are sending to only one recipient.
* 		You declare it like so:
* 			$to = array('recipient');
* 				or
* 			$to = array('one', 'two', 'three');
*		$sendMail = new Email($to, 'subject', 'message');
* 		if ($sendMail->send()) {
* 			// success
* 		} else {
* 			// failure
* 		}
* 		
*	To add various features (declare these before using $sendMail-send()):
* 		To add a CC address:
* 			$sendMail->setCC('email address');
* 		To add a BCC address:
* 			$sendMail->setBCC('email address');
* 		To set the from name:
* 			$sendMail->setFromName('name of sender');
* 		To set the from email:
* 			$sendMail->setFromEmail('email of sender');
* 		To set a content type (default is text/html):
* 			$sendMail->setContentType('content type');
* 		To set a charset (default is iso-8859-1):
* 			$sendMail->setCharset('charset');
*/

class Email
{

public $to = array();
public $subject;
public $message;
public $fromName;
public $fromEmail;
public $cc;
public $bcc;
public $contentType;
public $charset;
private $_headers;

public function __construct($to, $subject, $message) {
	if (!is_null($to) && !is_array($to)) {
		throw new Exception('The recipient names must be an array, even if there is only one recipient.');
	}
	if (is_null($to) || is_null($subject) || is_null($message)) {
		throw new Exception('There must be at least one recipient, a subject, and a message.');
	}
	$this->to = $to;
	$this->subject = $subject;
	$this->message = $message;
}

public function setCC($cc = NULL) {
	$this->cc = $cc;
}

public function setBCC($bcc = NULL) {
	$this->bcc = $bcc;
}

public function setFromName($fromName = 'Website Name') {
	$this->fromName = $fromName;
}

public function setFromEmail($fromEmail = 'admin@website.com') {
	$this->fromEmail = $fromEmail;
}

public function setContentType($contentType = 'text/html') {
	$this->contentType = $contentType;
}

public function setCharset($charset = 'iso-8859-1') {
	$this->charset = $charset;
}

private function _setHeaders() {
	$this->_headers = "Content-type: " . $this->contentType . "charset=" . $this->charset . "\r\n";
	$this->_headers .= "From: " . $this->fromName . "<" . $this->fromEmail . "> \r\n";
	if ($this->cc != NULL) {
		$this->_headers .= "CC: " . $this->cc . "\r\n";
	}
	if ($this->bcc != NULL) {
		$this->_headers .= "BCC: " . $this->bcc . "\r\n";
	}
}

public function send() {
	$this->_setHeaders();
	$this->setFromName();
	$this->setFromName();
	$sent = FALSE;
	foreach ($this->to as $recipient) {
		if (mail($recipient, $this->subject, $this->message, $this->_headers)) {
			$sent = TRUE;
			continue;
		}
	}
	if ($sent = TRUE) {
		return TRUE;
	} else {
		return FALSE;
	}
}

}

[awjudd]

First thing I see are all of the public member variables but then you have setters for them.  I would make them all private.

 

What happens if I want more than 1 CC / BCC email address?  Do I have to make the semi-colon separated list myself?  You are also forcing there to be a TO address.  What if there isn't (i.e. a mailing list where you don't want to show the members of it so everyone is BCC'd)?

 

You never call the setContentType nor the setCharset functions so they will always be empty.

 

Your send method:

- you call $this -> setFromName () twice and then you start cycling through everyone in the TO list ... why are they not all sent to with one email?

- if any 1 mail is sent succesfully the $sent flag will be set to TRUE and it will mark it as a success

- continue in foreach loop is useless there ... it will just jump up to the beginning of the loop anyways (no other code below it)

- if ( $sent = TRUE ) ... you are assigning TRUE to the value so that will always evaluate to true.  All you would really need there is return $sent; because it is already either TRUE / FALSE

 

Another small thing I noticed, no comments throughout the code ... especially on those public functions

 

Hope this helps.

 

~juddster

[the182guy]

1. Instead of having all the properties public, make them private or protected and have methods to return them if needed, rather than public (no point using setters if they are public anyway).

 

2. You might want to re-think your logic where you check if a mail is sent, because currently it doesn't make sense, if a mail fails then the next succeeds, your script won't notice and will act like both succeeded.

 

3. Remove the continue statement because it does nothing when used like that. continue is for moving onto the next iteration, which your script will do anyway without that.

 

4. In the send method, you're overwriting the from name and headers, so if the coder had set it, it would be lost.

 

5. Why are you calling setFromName() twice in the send method?

 

Not trying to rip it apart just giving some pointers.

[KevinM1]

A couple of things after a quick glance:

 

1. Make your fields private, or protected if you plan on extending the class.  You have setters in place already, enforce their use by keeping the fields from being directly accessible.

 

2. Your send() method, as it stands now, will return true if only one message was sent successfully.  What if some are sent successfully, and others aren't?

 

3. Since you're already hard coding default values for your charset, headers, etc., why not simply assign those values when you declare those fields?  That would save you from having to call their associated methods when sending the mail for no real reason.  If the values need to be changed, you'll still have the setters in place.

 

In other words, this:

 

class Email
{
private $to = array();
private $subject;
private $message;
private $fromName = 'Website Name';
private $fromEmail 'admin@website.com';
private $cc = null;
private $bcc = null;
private $contentType = 'text/html';
private $charset = 'iso-8859-1';
private $_headers;

        // constructor omitted because it would remain the same.

public function setCC($cc) {
	$this->cc = $cc;
}

public function setBCC($bcc) {
	$this->bcc = $bcc;
}

public function setFromName($fromName) {
	$this->fromName = $fromName;
}

public function setFromEmail($fromEmail) {
	$this->fromEmail = $fromEmail;
}

public function setContentType($contentType) {
	$this->contentType = $contentType;
}

public function setCharset($charset) {
	$this->charset = $charset;
}

private function _setHeaders() {
	$this->_headers = "Content-type: " . $this->contentType . "charset=" . $this->charset . "\r\n";
	$this->_headers .= "From: " . $this->fromName . "<" . $this->fromEmail . "> \r\n";
	if ($this->cc != NULL) {
		$this->_headers .= "CC: " . $this->cc . "\r\n";
	}
	if ($this->bcc != NULL) {
		$this->_headers .= "BCC: " . $this->bcc . "\r\n";
	}
}

public function send() {
	$this->_setHeaders();
	$sent = FALSE;
	foreach ($this->to as $recipient) {
		if (mail($recipient, $this->subject, $this->message, $this->_headers)) {
			$sent = TRUE;
		}
	}
	if ($sent = TRUE) {
		return TRUE;
	} else {
		return FALSE;
	}
}
}

[Buddski]

As juddster pointed out

You never call the setContentType nor the setCharset functions so they will always be empty

Give the contentType and charset variables default values within the class.

 

Also as a thought, if you are sending out 15 emails, if the first email sends, the $sent variable is set to TRUE, if the rest fail, the result will still be true.. in my book 1/15 is a fail. It might be an option to store the results of each mailout in an array so you can see the actual result.

 

Edit: I need to type faster..

[RyanMinor]

Thanks for all of the comments so far. I really appreciate the input. It's amazing what you miss when you have someone else look at your code. Most of those errors are obvious things, lol. Thanks again. I will rewrite after work and re-post.

[RyanMinor]

Here is a slightly improved version based on some of your suggestions. What I am confused about is how to send the email to each recipient while keeping a log of failed and successful ones. Any help is appreciated. Thanks!

 

<?php 
/**
*	To use this email class in it's most basic form:
* 
* 		$to must be an array even if you are sending to only one recipient.
* 		You declare it like so:
* 			$to = array('recipient');
* 				or
* 			$to = array('one', 'two', 'three');
*		$sendMail = new Email($to, 'subject', 'message');
* 		if ($sendMail->send()) {
* 			// success
* 		} else {
* 			// failure
* 		}
* 		
*	To add various features (declare these before using $sendMail-send()):
* 		To add a CC address:
* 			$sendMail->setCC('email address');
* 		To add a BCC address:
* 			$sendMail->setBCC('email address');
* 		To set the from name:
* 			$sendMail->setFromName('name of sender');
* 		To set the from email:
* 			$sendMail->setFromEmail('email of sender');
* 		To set a content type (default is text/html):
* 			$sendMail->setContentType('content type');
* 		To set a charset (default is iso-8859-1):
* 			$sendMail->setCharset('charset');
*/

class Email
{

private $_to 			= array();
private $_subject;
private $_message;
private $_cc 			= array();
private $_bcc 			= array();
private $_fromName 		= 'Website';
private $_fromEmail 	= 'admin@website.com';
private $_contentType 	= 'text/html';
private $_charset 		= 'iso-8859-1';
private $_headers;

/**
 * Constructor method sets values for recipients, the subject, and the message.
 * 
 * It also performs various checks to ensure proper data.
 */
public function __construct($to, $subject, $message) {
	if (!is_null($to) && !is_array($to)) {
		throw new Exception('The recipient names must be an array, even if there is only one recipient.');
	}
	if (is_null($to) || is_null($subject) || is_null($message)) {
		throw new Exception('There must be at least one recipient, a subject, and a message.');
	}
	$this->_to = $to;
	$this->_subject = $subject;
	$this->_message = $message;
}

/**
 * This function sets the CC values in the form of an array so you can send more than one CC.
 */
public function setCC($cc) {
	if (!is_null($cc) && !is_array($cc)) {
		throw new Exception('The CC names must be an array, even if there is only one.');
	}
	$this->_cc = $cc;
}

/**
 * This function sets the BCC values in the form of an array so you can send more than one BCC.
 */
public function setBCC($bcc) {
	if (!is_null($bcc) && !is_array($bcc)) {
		throw new Exception('The BCC names must be an array, even if there is only one.');
	}
	$this->_bcc = $bcc;
}

/**
 * This function sets a user-defined value for the from name.
 */
public function setFromName($fromName) {
	$this->_fromName = $fromName;
}

/**
 * This function sets a user-defined value for the from email ddress.
 */
public function setFromEmail($fromEmail) {
	$this->_fromEmail = $fromEmail;
}

/**
 * This function sets a user-defined content type.
 */
public function setContentType($contentType) {
	$this->_contentType = $contentType;
}

/**
 * This function sets a user-defined charset value.
 */
public function setCharset($charset) {
	$this->_charset = $charset;
}

/**
 * This function sets the headers.
 * 
 * It is called by the send() method prior to send the email(s).
 */
private function _setHeaders() {
	$this->_headers = "Content-type: " . $this->_contentType . "charset=" . $this->_charset . "\r\n";
	$this->_headers .= "From: " . $this->_fromName . "<" . $this->_fromEmail . ">\r\n";
	if ($this->_cc != NULL) {
		foreach ($this->_cc as $cc) {
			$this->_headers .= "CC: " . $cc . "\r\n";
		}
	}
	if ($this->_bcc != NULL) {
		foreach ($this->_bcc as $bcc) {
			$this->_headers .= "BCC: " . $bcc . "\r\n";
		}
	}
}

/**
 * This function sends the email(s) to all recipient(s).
 */
public function send() {
	$this->_setHeaders();
	$sent = FALSE;
	foreach ($this->to as $recipient) {
		if (mail($recipient, $this->_subject, $this->_message, $this->_headers)) {
			$sent = TRUE;
		}
	}
	if ($sent) {
		return TRUE;
	} else {
		return FALSE;
	}
}

}

[Buddski]

Here is a basic example for you.

Keep in mind you can determine the return value however you wish, Ive just set it to return false if $sent is empty.

public function send() {
$sent = $failed = array();
$this->_setHeaders();
foreach ($this->to as $recipient) {
	if (mail($recipient, $this->_subject, $this->_message, $this->_headers)) {
		$sent[] = $recipient;
	} else {
		$failed[] = $recipient;
	}
}
return !empty($sent);
}

[RyanMinor]

Excellent, I have it working now. Thanks a lot and thanks to everyone who took the time to throw me some pointers. I really appreciate it.