OOP simple contact form

Question

Here is my Code. But I feel that my code isn't really that much object oriented.

There is a small piece of "styling" in it as well, the "message error" thing at the end.

Should that be in my class or should it go somewhere else?

<?php
require 'contact.class.php';
?>
<form action="" method="post">
<?php
if (isset($_POST["submit"])) {
$sendMail = new gw2Mail();
$sendMail->senderName = $_POST['senderName'];
$sendMail->senderEmail = $_POST['senderEmail'];
$sendMail->recipient = $_POST['recipient'];
$sendMail->copy = $_POST['copy'];
$sendMail->subject = $_POST['subject'];
$sendMail->message = $_POST['message'];
$sendMail->sendMail();
}
?>
<table class='ipb_table ipsMemberList'>
 <tr class='header' colspan='4'>
 <th scope='col' colspan='5'>Send E-mail</th>
 </tr>
 <tr class='row1'>
 <td style='width: 50%'><strong>From:</strong><br />Enter the sender's name in this field.</td>
 <td><input type="text" class="input_text" name="senderName" id="senderName" value="<?php if (isset($_POST['senderName'])) { echo $_POST['senderName']; } ?>" size="50" maxlength="125" /></td>
 </tr>
 <tr class='row2'>
 <td style='width: 50%'><strong>From E-mail:</strong><br />Enter the sender's e-mail address in this field.</td>
 <td><input type="text" class="input_text" name="senderEmail" id="senderEmail" value="<?php if (isset($_POST['senderEmail'])) { echo $_POST['senderEmail']; } ?>" size="50" maxlength="125" /></td>
 </tr>
 <tr class='row1'>
 <td style='width: 50%'><strong>Recipient:</strong><br />Enter the recipient's e-mail address in this field.</td>
 <td><input type="text" class="input_text" name="recipient" id="recipient" value="<?php if (isset($_POST['recipient'])) { echo $_POST['recipient']; } ?>" size="50" maxlength="125" /></td>
 </tr>
 <tr class='row2'>
 <td style='width: 50%'><strong>Carbon Copy:</strong><br />Send a copy to someone else? Enter another e-mail address here. Leave blank for no copy.</td>
 <td><input type="text" class="input_text" name="copy" id="copy" value="<?php if (isset($_POST['copy'])) { echo $_POST['copy']; } ?>" size="50" maxlength="125" /></td>
 </tr>
 <tr class='row1'>
 <td style='width: 50%'><strong>Subject:</strong><br />Enter a subject in this field.</td>
 <td><input type="text" class="input_text" name="subject" id="subject" value="<?php if (isset($_POST['subject'])) { echo $_POST['subject']; } ?>" size="50" maxlength="50" /></td>
 </tr>
 <tr class='row2'>
 <td colspan='2' style='width: 100%'><textarea style="height: 250px; width: 99%;" name="message" id="message" cols="30" rows="14" virtual wrap="on"><?php if (isset($_POST['message'])) { echo $_POST['message']; } ?></textarea></td>
 </tr>
</table>
<input type="submit" name="submit" value="Submit" tabindex="50" class="input_submit" accesskey="s" />
</form>

And this is the contact.class.php:

<?php
class gw2Mail {
 var $senderName;
 var $senderEmail;
 var $recipient;
 var $copy;
 var $subject;
 var $message;
 var $bcc;
 public function sendMail()
 {
 if ($this->senderName != "") {
 $this->senderName = filter_var($this->senderName, FILTER_SANITIZE_STRING);
 if ($this->senderName == "") {
 $errors .= '- Please enter a valid name!';
 }
 } else {
 $errors .= '- You forgot to enter a name!<br />';
 }
 if ($this->senderEmail != "") {
 $this->senderEmail = filter_var($this->senderEmail, FILTER_SANITIZE_STRING);
 if ($this->senderEmail == "") {
 $errors .= '- Please enter a valid Email!';
 }
 } else {
 $errors .= '- You forgot to enter an email!<br />';
 }
 if ($this->recipient != "") {
 $this->recipient = filter_var($this->recipient, FILTER_SANITIZE_STRING);
 if ($this->recipient == "") {
 $errors .= '- Please enter a valid recipient email!';
 }
 } else {
 $errors .= '- You forgot to enter a recipient email!<br />';
 }
 if ($this->subject != "") {
 $this->subject = filter_var($this->subject, FILTER_SANITIZE_STRING);
 if ($this->subject == "") {
 $errors .= '- Please enter a valid subject!';
 }
 } else {
 $errors .= '- You forgot to enter a subject!<br />';
 }
 if ($this->message != "") {
 $this->message = filter_var($this->message, FILTER_SANITIZE_STRING);
 if ($this->message == "") {
 $errors .= '- Please enter a valid message!';
 }
 } else {
 $errors .= '- You forgot to enter a message!<br />';
 }
 if (!$errors) {
 $this->bcc="";
 $headers = "From: $this->senderName <$this->senderEmail>";
 $headers .= "\r\nCc: $this->copy";
 $headers .= "\r\nBcc: $this->bcc\r\n\r\n";
 $send_contact=mail("$this->recipient","$this->subject","$this->message","$headers");
 } else {
 echo '<p class=\'message error\'>';
 echo '<font color="#FFFFFF">' . $errors . '</font>';
 echo '</p><br />';
 }
 }
 }
?>

Answer 1

Here are some observations noticed:

For something to be OOP it needs to be somewhat reusable. As always use SOLID principles to achieve this.

Because you are using styling it broke some of the rules. Consider returning outputs such as errors and such to be pure text without styling and let the return handle it. Reason: what happens if you want to log the fail message and shoot it out internally or to a log file.

Your sendmail is a jack of all trade: it does the header, store the emails, AND checks for errors - this is procedural (start, middle end) - consider separating it into different functions of class (your original one and then have a separate class to do validations which you send as arguments).

Consider using a constructor. You initialize the object with settings and then reuse the function.

Another point to add: attributes (or variables from a class that pertains to the class) should never be never accessible to the "main" or elsewhere but to the class itself. Use accessor functions like get/set or magic functions _get _set instead. Classes are suppose to be encapsulated so that outside code cannot effected without going through a checker. I'm aware that its easier to just access it directly but you defeat the purpose of OOP in that sense.

Lastly, too many if/else renders the code too tight which is why I suggested the validation class - let that be the class to check on the arguments rather than the mailer itself.

Answer 2

1. The result is unused here:

   $send_contact=mail("$this->recipient","$this->subject","$this->message","$headers");
   

   mail returns TRUE if the mail was successfully accepted for delivery and FALSE otherwise. You could show that to the user.

2. $sendMail->senderName = $_POST['senderName'];
   $sendMail->senderEmail = $_POST['senderEmail'];
   $sendMail->recipient = $_POST['recipient'];
   $sendMail->copy = $_POST['copy'];
   $sendMail->subject = $_POST['subject'];
   $sendMail->message = $_POST['message'];
   

   I've found this kind of indentation hard to maintain. If you have a new variable with a longer name you have to modify several other lines too to keep it nice. It could also cause unnecessary patch/merge conflicts.

3. $errors .= '- Please enter a valid name!';
   

   How does a valid name look like? Help users with detailed error messages.

4. $errors .= '- You forgot to enter an email!<br />';
   

   Error messages shouldn't blame the user (don't say what he did is wrong), give a suggestion about what they should do. (See: Should we avoid negative words when writing error messages?, What will be the Best notifications and error messages?)

• azngunit81: I think we also miss one more thing about oop - that his attributes are set to public when it should be private.
• palacsint: @azngunit81: Yes, I guess you're right, feel free to edit your answer. I'm not too familar with OOP in PHP so it was just a few notes about the code. (According to the help center, reviewers may comment on any part of the code.)
• azngunit81: OOP is general subject let it be C++(where i learned it from) to java and php (there is even OOP in javascript imagine that)