3
\$\begingroup\$

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.' );
}
\$\endgroup\$
  • \$\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 Oct 1 '15 at 7:07
2
\$\begingroup\$

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,

  1. Much cleaners and improves readability
  2. When you need double quotes inside the string you don't need to escape them
  3. 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.

\$\endgroup\$
  • \$\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 Oct 3 '15 at 16:58
  • \$\begingroup\$ Hi @Zak. I have edited my answer with how & why you should use it. \$\endgroup\$ – blackpla9ue Oct 4 '15 at 3:39
1
\$\begingroup\$

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 the if 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);
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.