patternphpMinor
Recursive Directory Copy
Viewed 0 times
recursivecopydirectory
Problem
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:
```
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
Here's the code.
Here's the main PHP script:
```
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
Solution
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)
It is not necessary to put quotes around variables unless you are concatenating them. By the way $username is not set in your script.
This will cause an error notice if firstname is not set, check it exists like this
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
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
Code Snippets
// $dbunames = mysql_query("SELECT * FROM users WHERE website='$website'");
$dbunames = mysql_query("SELECT * FROM users WHERE website='$website'") or die(mysql_error());// mysql_connect("$host", "$username", "$password") or die("cannot connect");
mysql_connect($host, $username, $password) or die("cannot connect");// $firstname = $_POST['firstname'];
$firstname = isset($_POST['firstname']) ? $_POST['firstname'] : '';Context
StackExchange Code Review Q#30472, answer score: 7
Revisions (0)
No revisions yet.