Jump to content

Code Critique Please!

RPMiSO

By RPMiSO
in PHP Coding Help

 Share

Recommended Posts

RPMiSO Newbie

RPMiSO

Posted
Posted

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();

?>

Link to comment
Share on other sites
Mchl Regular Member

Mchl

Posted
Posted

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 {...}

 

 

 

Link to comment
Share on other sites
ignace Advanced Member

ignace

Posted
Posted

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) {

Link to comment
Share on other sites
This thread is more than a year old. Please don't revive it unless you have something important to add.

Join the conversation

You can post now and register later. If you have an account, sign in now to post with your account.

Guest
Reply to this topic...

×   Pasted as rich text.   Restore formatting

  Only 75 emoji are allowed.

×   Your link has been automatically embedded.   Display as a link instead

×   Your previous content has been restored.   Clear editor

×   You cannot paste images directly. Upload or insert images from URL.

×
 Share
Go to topic listing
×
×
  • Create New...

Important Information

We have placed cookies on your device to help make this website better. You can adjust your cookie settings, otherwise we'll assume you're okay to continue.