Recursive Directory Copy

Question

I've been told that my code is horribly-written. It works exactly the way I want it to, but I have been told that it needs to be safer and more efficient.

Here's the code.

Here's the main PHP script:

<?php
$host     = "*******"; // Host firstname
$name     = "*******"; // Mysql userfirstname
$password = "*******!"; // Mysql password
$db_name  = "*******"; // Database firstname
$tbl_name = "users"; // Table firstname
// Connect to server and select database.
mysql_connect("$host", "$username", "$password") or die("cannot connect");
mysql_select_db("$db_name") or die("cannot select DB");
foreach($_POST as &$v)
    $v = mysql_real_escape_string($v);
// Get values from form
$firstname         = $_POST['firstname'];
$lastname          = $_POST['lastname'];
$email             = $_POST['email'];
$resumewebsitename = $_POST['resumewebsitename'];
$resumewebsitename = substr($resumewebsitename, 0, 20);
$website           = 'http://www.thorbis.com/onlineresumes/users/' . $resumewebsitename;
$cell              = $_POST ['cell'];
$overview          = $_POST ['overview'];
$query = mysql_query("SELECT * FROM users WHERE email='$email'");
if(mysql_num_rows($query) != 0)
{
    echo "email already exists";
    // redirect back to form and populate with 
    // data that has already been entered by the user
}
else

{
                $dbunames = mysql_query("SELECT * FROM users WHERE website='$website'");
                if (mysql_num_rows($dbunames) > 0) {
                                echo "Sorry That Resume Name Has Been Taken Please Try Again :D";
                } //mysql_num_rows($dbunames) > 0
                else {
                                // function to recursively copy
                                // a directory and its subdirectories
                                function copyRecursive($source, $destination)
                                {
                                                // check if source exists
                                                if (!file_exists($source)) {
                                                                die("'$source' is not valid");
                                                } //!file_exists($source)
                                                if (!is_dir($destination)) {
                                                                mkdir($destination);
                                                } //!is_dir($destination)
                                                // open directory handle
                                                $dh = opendir($source) or die("Cannot open directory '$source'");
                                                // iterate over files in directory
                                                while (($file = readdir($dh)) !== false) {
                                                                // filter out "." and ".."
                                                                if ($file != "." && $file != "..") {
                                                                                if (is_dir("$source/$file")) {
                                                                                                // if this is a subdirectory
                                                                                                // recursively copy it
                                                                                                copyRecursive("$source/$file", "$destination/$file");
                                                                                } //is_dir("$source/$file")
                                                                                else {
                                                                                                // if this is a file
                                                                                                // copy it
                                                                                                copy("$source/$file", "$destination/$file") or die("Cannot copy file '$file'");
                                                                                }
                                                                } //$file != "." && $file != ".."
                                                } //($file = readdir($dh)) !== false
                                                // close directory
                                                closedir($dh);
                                }
                                $source_directory      = "Interactive Resume/";
                                $destination_directory = "users/";
                                copyRecursive($source_directory, $destination_directory);
                                @rename("users/user", "users/" . $resumewebsitename);
                                $emails = ("$email, [email protected]");
                                $to      = $emails;
                                $subject = 'Thorbis | Submit';
                                $message = "<h3 style='color: red;'>This is ALL of your Info That You Inputed I will get back to you ASAP and customly make your website for you :D if i dont email me again at <a style='color: darkread;' href='mailto:[email protected]'>[email protected]</a></h3><br>
                    <a href='https://plus.google.com/113626141520121345204/posts' style='color:blue;'>Follow Me On google Plus</a><br>
                    <a href='http://twitter.com/thorbis' style='color:blue;'>Follow Me On Twitter</a><br>
                    <a href='http://www.linkedin.com/profile/view?id=226237754&goback=%2Enmp_*1_*1_*1_*1_*1_*1_*1_*1_*1_*1&trk=spm_pic' style='color:blue;'>Find Me on Linked In</a><br>
                    <a href='http://facebook.com/thorbisinc' style=' color:blue;'>Follow Me On Facebook</a><br>
                    <a href='http://www.youtube.com/user/byronwade10' style=' color:blue;'>Subscrib To Me On YouTube</a><br>
                    <a href='https://github.com/Thorbis/Rejoice' style=' color:blue;'>Fork Me On GitHub</a></br><hr>" . "Name: " . $firstname . "<br>" . "Last: " . $lastname . "<br>" . "Email: " . $email . "<br>" . "cell: " . $cell . "<br>" . "Website: <a href='" . $website . "'>CLIENT WEBSITE<a/><br>";
                                $headers = "From: $from\r\n";
                                $headers .= "Content-type: text/html\r\n";
                                if (mail($to, $subject, $message, $headers)) {
                                                echo ("<p>Since You Have Used My services I will get an email stating that you have filled out this form and i will contact you back within 6 hours of you submiting the form! Also you sill get an adutomated message of exactly what i get!</p>");
                                } //mail($to, $subject, $message, $headers)
                                else {
                                                echo ("<p>I dident get a message stating that you have filled out the form can you manualy send me your email and info so i can customize your website please my email is [email protected] thank you :D</p>");
                                }
                                // Insert data into mysql
                                $sql    = "INSERT INTO $tbl_name(firstname, lastname, email, website, cell, overview)VALUES('$firstname', '$lastname', '$email', '$website', '$cell', '$overview')";
                                $result = mysql_query($sql);
                                // if successfully insert data into database, displays message "Successful".
                                if ($result) {
                                                echo "Successful";
                                                echo "<BR>";
                                                echo "This is your new resume site i will edit it with your info ASAP once i have contected you: <a href='" . $website . "'>Your Website</a>";
                                                echo "<BR>";
                                                echo "<a href='http://www.thorbis.com'>Back to Home Page</a>";
                                } //$result
                }
}
?>

Answer 1

There is quite a lot that can be done

For a start use mysqli_ functions as mysql_ functions are deprecated

on database queries, it is good practice to check for errors (like this)

// $dbunames = mysql_query("SELECT * FROM users WHERE website='$website'");
$dbunames = mysql_query("SELECT * FROM users WHERE website='$website'") or die(mysql_error());

It is not necessary to put quotes around variables unless you are concatenating them. By the way $username is not set in your script.

// mysql_connect("$host", "$username", "$password") or die("cannot connect");
mysql_connect($host, $username, $password) or die("cannot connect");

This will cause an error notice if firstname is not set, check it exists like this

// $firstname         = $_POST['firstname'];
$firstname         = isset($_POST['firstname']) ? $_POST['firstname'] : '';

My personal preference is not to nest functions inside if/else statements as you have done with the copyRecursive($source, $destination) function. Put functions at the top level it will make your code easier to read.

For code readability I would also move your email generation code into a separate function

Answer 2

I agree with most of the points in @bumperbox's answer, except handling errors with ... or die. That's not very graceful. It's better to use if-then-else blocks and print informative error messages for the user, and log more details for yourself. Error messages for the user should be human-friendly and without revealing sensitive internal details of your setup.

Move copyRecursive and the email sending part to top-level functions, possible in a reusable utility file that you can include when needed and reuse easily.

I recommend making the most important literal strings constants and define them at the top of the file. I would do the same for SQL statements, making them parameterized using %s and %d as appropriate. That way you can see easily what SQL statements are used by the code, all in one place at the top.

Clean up the indenting. You are using sometimes 4 spaces sometimes 16. I would go with 4 spaces consistently, to make long lines easier to read without scrolling.

Answer 3

In addition to the answers provided above, removing the PHP closing tag is also considered good practice, at least for beginners. Whitespace following the closing tag, whether introduced by the developer, user, or an FTP application, can cause unwanted output, PHP errors, or if the latter are suppressed, blank pages.

Alternatively, you may want to use a comment block to mark the end of your file so you can still identify the file as being complete, and not truncated.

Here's a little something on the topic: https://stackoverflow.com/questions/4410704/why-would-one-omit-the-close-tag