This is a simple web form handler in PHP that sends a completely filled web form to a email.
- I see lots of repeating code. What is the best way to reduce repetition?
- Is my way of checking for fields being empty good? The "empty()" method seems to also treat "0", "0.0" and similar values as empty.
- Am I doing something that is unneccesary?
Any other criticism is appreciated.
<?php
if ( $_SERVER['REQUEST_METHOD'] !== 'POST' ) {
die( 'Please use a form to send a message!' );
}
if ( !isset( $_POST['name'] ) || trim( $_POST['name'] ) === '' ) {
die( 'Please enter your name!' );
}
if ( !isset( $_POST['phone'] ) || trim( $_POST['phone'] ) === '' ) {
die( 'Please enter your phone number!' );
}
if ( !isset( $_POST['city'] ) || trim( $_POST['city'] ) === '' ) {
die( 'Please enter your city!' );
}
$name = trim( $_POST['name'] );
$phone = trim( $_POST['phone'] );
$city = trim( $_POST['city'] );
$contents = '';
$contents .= "New order!\r\n";
$contents .= "\r\n";
$contents .= "Name: {$name}\r\n";
$contents .= "Phone number: {$phone}\r\n";
$contents .= "City: {$city}\r\n";
if ( mail( '<email removed>', 'New order!', htmlspecialchars($contents), 'From: <email removed>' ) ) {
die( 'Thanks! We will contact you very soon!' );
} else {
die( 'Unfortunately an error occurred. Please try again later.' );
}
-
\$\begingroup\$ Unfortunately, editing revisions of your code into the question is strictly off-topic, however, you could consider instead posting it as a new question. \$\endgroup\$Quill– Quill2015年10月01日 07:07:29 +00:00Commented Oct 1, 2015 at 7:07
2 Answers 2
To add to Quill's answer, use heredoc whenever you want to use multiline strings.
$contents = <<<CON
New order!
Name: {$name}
Phone number: {$phone}
City: {$city}
CON;
$content = htmlspecialchars($contents);
EDIT
Why & how it improves your code,
- Much cleaners and improves readability
- When you need double quotes inside the string you don't need to escape them
- You don't have to worry about which characters to use when you need a line break.
A common use of this is when you write SQL queries, HTML or Email message bodies in your application. Basically multiline strings.
-
\$\begingroup\$ Hi and welcome to Code Review, could you please expand a bit on why and how your suggestion is an improvement in this (and similar) situations. \$\endgroup\$Kaz– Kaz2015年10月03日 16:58:03 +00:00Commented Oct 3, 2015 at 16:58
-
\$\begingroup\$ Hi @Zak. I have edited my answer with how & why you should use it. \$\endgroup\$Malitta N– Malitta N2015年10月04日 03:39:00 +00:00Commented Oct 4, 2015 at 3:39
There's a few things that could be improved:
Spacing:
Don't put extraneous space/padding in your brackets, it's extraneous at best.
if ( $_SERVER['REQUEST_METHOD'] !== 'POST' ) { die( 'Please use a form to send a message!' ); }
should look this instead:
if ($_SERVER['REQUEST_METHOD'] !== 'POST') {
die('Please use a form to send a message!');
}
The four opening if
statements' contents are double intented, this is an incorrect level of indentation.
$contents = ''; $contents .= "New order!\r\n"; $contents .= "\r\n"; $contents .= "Name: {$name}\r\n"; $contents .= "Phone number: {$phone}\r\n"; $contents .= "City: {$city}\r\n"; if ( mail( '<email removed>', 'New order!', htmlspecialchars($contents), 'From: <email removed>' ) ) { die( 'Thanks! We will contact you very soon!' ); } else { die( 'Unfortunately an error occurred. Please try again later.' ); }
A few points to make about this:
- The opening
$contents
declaration is both useless and extraneous. - The
mail
call inside theif
statement should be moved to a variable. - The
mail
call's parameters should be moved to variables too.
Reducing that, the if
statement can be converted to a simple ternary, effectively rendering:
$contents = "New order!\r\n";
$contents .= "\r\n";
$contents .= "Name: {$name}\r\n";
$contents .= "Phone number: {$phone}\r\n";
$contents .= "City: {$city}\r\n";
$contents = htmlspecialchars($contents);
$toEmail = '<email removed>';
$fromEmail = 'From: <email removed>';
$title = 'New order!';
$successMessage = 'Thanks! We will contact you very soon!';
$failureMessage = 'Unfortunately an error occurred. Please try again later.';
$mail = mail($toEmail, $title, $contents, $fromEmail);
die($mail ? $successMessage : $failureMessage);