Code Critique Please!

[RPMiSO]

Hello,

 

I'm currently attempting to learn php and I've gotten to a point where I am able to build functional scripts but I'm still looking for more.

 

I'm trying to improve the quality of my scripts and learn new things so that I can become better programmer.

 

I've been hunting around the internet looking for advice on how to become a better programmer as I'm aware that programming can't be mastered just by reading some books (although It certainly helps).

 

I read some where that it's beneficial to take part in a community and allow other coders to read your code and offer their advice, so by following that suggestion I've found myself here.

 

My script isn't complete but it is working -- I'd just like to hear your suggestions for improvement and what to learn/do next.

 

Thanks, Dan.

 

I just had a look through other members that have posted code in a zip file and it seems that they have not got any replies. I understand that it's a load of hassle.

 

list_files.php

include 'config.php';
include 'functions.php';
include 'db_connect.php';


$page = $_GET['page']; //get page number
$action = $_GET['action'];

if ($action == "list"){
    if (!isset($page)){$page = 0;} //if no page number set 0
    if (isset($page)){
        $query = "SELECT * from files LIMIT " . $page * $maxresult . "," . $maxresult;
        if ($result = mysqli_query($link,$query)){
            while ($row = mysqli_fetch_assoc($result)){
                echo "<a href=\"" . $_SERVER[’PHP_SELF’] . "?action=listen&uid=".$row['uid']."\">".$row['filename']."</a><br />";
            }
            mysqli_free_result($result);
        }
        $query = "SELECT * from files";
        if ($result = mysqli_query($link,$query)){
            $pagecount = round(mysqli_num_rows($result) / $maxresult,0,PHP_ROUND_HALF_DOWN);
            mysqli_free_result($result);
            for ($i = 0; $i <= $pagecount; $i++){
                if ($i != $page){
                    echo "<a href=\"". $_SERVER[’PHP_SELF’] . "?action=$action&page=$i\">$i</a> - ";
                } else {
                    echo "<b>$i</b> - ";
                }
            }
        }
    }
} elseif ($action == "listen"){
    $uid = $_GET['uid'];
    $query = "SELECT * from files WHERE uid='$uid'";
    if ($result = mysqli_query($link,$query)){
    
        $row = mysqli_fetch_assoc($result);
        
        echo $row['filename']."<BR />";
        echo "<object type=\"application/x-shockwave-flash\" data=\"player_mp3.swf\" width=\"200\" height=\"20\">"
             ."<param name=\"movie\" value=\"player_mp3.swf\" />"
             ."<param name=\"bgcolor\" value=\"#000000\" />"
             ."<param name=\"FlashVars\" value=\"mp3=http://". $_SERVER['HTTP_HOST'] ."$curdir$mp3directory" . $row['location'] . "\" />"
             ."</object>";
        mysqli_free_result($result);
    }
}
include 'db_close.php';
?> 
<a href="javascript:history.back()">Go back</a>

 

process_upload.php

<?php
include 'config.php';
include 'functions.php';
set_time_limit(0);



//Check for .mp3 extension
if (substr($_FILES['userfile']['name'],strlen($_FILES['userfile']['name']) - 4,strlen($_FILES['userfile']['name'])) == ".mp3"){
    
    $subdirectory = 1;
    $uploaddir    = $base_path . $mp3directory . $subdirectory . DIRECTORY_SLASH;
    $uploadname   = randomString(24) . ".mp3";
    
    while(countDir($uploaddir) >= 10){
        if (is_dir($uploaddir) == true){
            if (countDir($uploaddir) >= 10){
                $subdirectory++;
                $uploaddir = $base_path . $mp3directory . $subdirectory . DIRECTORY_SLASH;
                if(!is_dir($uploaddir)){mkdir($uploaddir);}
                }
            }
            $uploaddir = $base_path . $mp3directory . $subdirectory . DIRECTORY_SLASH;
        }
        
    if (move_uploaded_file($_FILES['userfile']['tmp_name'], $uploaddir . $uploadname)){
        echo "Success!\n";
        include 'db_connect.php';
        addFile($link,str_replace(".mp3","",$_FILES['userfile']['name']),"/$subdirectory/$uploadname");
        include 'db_close.php';
        createZip($uploaddir . $uploadname,$_FILES['userfile']['name']);
    } else {
        echo "Fail!\n";
        echo $_FILES['userfile']['error'];
    }
    
} else {
    echo "Invalid File Extension<BR />";
}
    

 

functions.php

<?php
// General Functions
function randomString($length){
    $characters = array("a","b","c","d","e","f","g","h","i","j","k","l","m","n","o","p","q","r","s","t","u","v","w","x","y","z",1,2,3,4,5,6,7,8,9,0);
    for ($i = 0; $i < $length; $i++){
        $random_string .= $characters[rand(0,sizeof($characters))];
    }
    return $random_string;
}

function countDir($directory){
    $files = scandir($directory);
    foreach($files as $file){
        if (!($file == ".") && !($file == "..")){ 
            $count++;
        }
    }
    return $count;
}

//Database Functions
function addFile($link,$filename,$location){
    if ($result = mysqli_query($link,"INSERT INTO files (uid,filename,location,date) VALUES('','$filename','$location','')")){
    }
    
}
function createZip($mp3path,$filename){
    date_default_timezone_set('UTC');
    $zip = new ZipArchive;
    $zipdir = $_SERVER['DOCUMENT_ROOT'] . dirname($_SERVER['SCRIPT_NAME']) . "/zip/" . date('dm') . "/";
    if(!is_dir($zipdir)){mkdir($zipdir);}
    $res = $zip->open($zipdir . str_replace(".mp3",".zip",$filename), ZipArchive::CREATE);
    if ($res === true) {
        $zip->addFile($mp3path,$filename);
        $zip->setArchiveComment('Downloaded using empeethree.com');
        $zip->close();
        echo 'ok';
    } else {
        echo 'failed';
    }
}



?>

 

config.php

<?php
//config file
define("MAX_FILE_DIR",1000);
define("SQL_HOST","localhost");
define("SQL_USER","root");
define("SQL_PASS","");
define("SQL_DB","empeethree");
define("DIRECTORY_SLASH","/");
$maxresult = 10;
$subdirectory = 1; //default subdirectory number
$mp3directory = "/mp3/"; //script directory
$curdir = dirname($_SERVER['SCRIPT_NAME']);
$base_path = $_SERVER['DOCUMENT_ROOT'] . $curdir;
//phpinfo();

?>

[Mchl]

You could possibly refactor randomString() function using a range of ASCII codes and chr function instead of this ugly array.

 

addFile() is vulnerable to SQL injection. While we're at it, almost all other queries in your script are also vulnerable. You should use mysqli_real_escape_string whenever you interpolate user supplied variables into query strings.

 

This

if (!isset($page)){$page = 0;} //if no page number set 0
    if (isset($page)) {...}

 

would be more clear as

 

if (!isset($page)){$page = 0;} //if no page number set 0
    else {...}

 

 

 

[ignace]

Another piece of good advice: know your language (and it's library)

 

For example:

define("DIRECTORY_SLASH","/");

 

Is actually already defined as DIRECTORY_SEPARATOR

 

function countDir($directory){
    $files = scandir($directory);
    foreach($files as $file){
        if (!($file == ".") && !($file == "..")){ 
            $count++;
        }
    }
    return $count;
}

 

Just feels wrong. You could easily read a directory and then just perform a count or the prefered sizeof

 

if ($result = mysqli_query($link,"INSERT INTO files (uid,filename,location,date) VALUES('','$filename','$location','')")){
    }

 

Instead just

 

mysqli_query($link,"INSERT INTO files (uid,filename,location,date) VALUES('','$filename','$location','')"));

 

function createZip($mp3path,$filename){
    date_default_timezone_set('UTC');
    $zip = new ZipArchive;
    $zipdir = $_SERVER['DOCUMENT_ROOT'] . dirname($_SERVER['SCRIPT_NAME']) . "/zip/" . date('dm') . "/";
    if(!is_dir($zipdir)){mkdir($zipdir);}
    $res = $zip->open($zipdir . str_replace(".mp3",".zip",$filename), ZipArchive::CREATE);
    if ($res === true) {
        $zip->addFile($mp3path,$filename);
        $zip->setArchiveComment('Downloaded using empeethree.com');
        $zip->close();
        echo 'ok';
    } else {
        echo 'failed';
    }
}

 

When you are using echo inside a function then you are missing the point. A function should only return something because you may find you one day using createZip() but are then confronted with it's output that you in this case do not want. Instead use something like:

 

define('F_CREATE_ZIP_SUCCESS', 1);
define('F_CREATE_ZIP_FAILURE', 2);

function createZip($mp3path,$filename){
    date_default_timezone_set('UTC');
    $zip = new ZipArchive;
    $zipdir = $_SERVER['DOCUMENT_ROOT'] . dirname($_SERVER['SCRIPT_NAME']) . "/zip/" . date('dm') . "/";
    if(!is_dir($zipdir)){mkdir($zipdir);}
    $res = $zip->open($zipdir . str_replace(".mp3",".zip",$filename), ZipArchive::CREATE);
    if ($res === true) {
        $zip->addFile($mp3path,$filename);
        $zip->setArchiveComment('Downloaded using empeethree.com');
        $zip->close();
        return F_CREATE_ZIP_SUCCESS;
    } else {
        return F_CREATE_ZIP_FAILED;
    }
}

if (createZip(..) === F_CREATE_ZIP_FAILED) {
    ..

 

However this is a bad example because you actually only have 2 possibilities where you would then better use return true & return false respectively ok and failed.

 

Also worth noting is that the execution of a function stops after a return so

 

return true;
    } else {
        return false;
    }

 

Can be written as:

 

return true;
}
return false;

 

(substr($_FILES['userfile']['name'],strlen($_FILES['userfile']['name']) - 4,strlen($_FILES['userfile']['name'])) == ".mp3")

 

This is what I meant by know your library as the same can be written as:

 

('mp3' == pathinfo($_FILES['userfile']['name'], PATHINFO_EXTENSION))

 

function randomString($length){
    $characters = array("a","b","c","d","e","f","g","h","i","j","k","l","m","n","o","p","q","r","s","t","u","v","w","x","y","z",1,2,3,4,5,6,7,8,9,0);
    for ($i = 0; $i < $length; $i++){
        $random_string .= $characters[rand(0,sizeof($characters))];
    }
    return $random_string;
}

 

Like Mchl already said should you refactor randomString() I would recommend rewriting it

 

function generateString($length = 16) {
    $hash = array_merge(range('A', 'Z'), rand('a', 'z'), range(0, 9));
    return implode('', array_rand($hash, $length));
}

 

Yes this function does exactly the same.

 

while(countDir($uploaddir) >= 10){

 

!!HORROR!!

 

$files = scandir($uploaddir);
$sizeof = sizeof($files);
while ($sizeof >= 10) {
  ..
  --$sizeof;
}

 

while(countDir($uploaddir) >= 10){
        if (is_dir($uploaddir) == true){
            if (countDir($uploaddir) >= 10){
                $subdirectory++;
                $uploaddir = $base_path . $mp3directory . $subdirectory . DIRECTORY_SLASH;
                if(!is_dir($uploaddir)){mkdir($uploaddir);}
                }
            }
            $uploaddir = $base_path . $mp3directory . $subdirectory . DIRECTORY_SLASH;
        }

 

There are no words for this massacre..

 

$page = $_GET['page']; //get page number
$action = $_GET['action'];

if ($action == "list"){
    if (!isset($page)){$page = 0;} //if no page number set 0

 

Instead write:

 

$page = isset($_GET['page']) ? intval($_GET['page']) : null;
$action = $_GET['action'];

if ('list' == $action) {
   if ($page) {

[RPMiSO]

Thanks for the advice. I'll take all of that on board and try again.