New to PHP - Criticize my code and tell me how to improve

PHP
1

I’ve made a script that will list directories and pdf files on our intranet. Everything is working so I don’t need anything debugged. I was basically just wondering if I’ve violated any best practices or how I could improve on things. Did I do anything that isn’t considered good programming?

The script allows users who have access to the folder structure to create and delete new folders and update pdf documents.

I’ve replaced confidential paths with the word “example”


<?php

if (!isset($_GET['dir'])) { // code to execute if no directory has been chosen

	$search = $_SERVER['DOCUMENT_ROOT'] . '/data/docs/example/';
	$folders = str_replace($search, '', glob($search . '*', GLOB_ONLYDIR));
	$files = str_replace($search, '', glob($search . '*.[pP][dD][fF]'));

	$ii = count($folders);
	for ($i = 0; $i < $ii; $i++) {
		echo '<a href="?dir[]=' . rawurlencode($folders[$i]) . '"><img src="http://example.com/data/images/folder.png" />&nbsp;' . $folders[$i] . '</a><br />';
	}

	$ii = count($files);
	for ($i = 0; $i < $ii; $i++) {
		echo '<a href="http://example.com/data/docs/example/' . rawurlencode($files[$i]) . '"><img src="http://example.com/data/images/reader.png" />&nbsp;' . $files[$i] . '</a><br />';
	}

}

if (isset($_GET['dir'])) { // code to execute when directory has been chosen

	foreach ($_GET['dir'] as $dir) {
		$dirs[] = strip_tags(rawurldecode($dir));
	}

	foreach ($dirs as $dir) {
		$path .= $dir . '/';
	}

	$search = $_SERVER['DOCUMENT_ROOT'] . '/data/docs/example/' . $path;

	$folders = str_replace($search, '', glob($search . '*', GLOB_ONLYDIR));
	$files = str_replace($search, '', glob($search . '*.[pP][dD][fF]'));

	$outDirs = explode('/', $path, -1);
	$fileLink = 'http://example.com/data/docs/example/';
	
	$ii = count($outDirs);
	for ($i = 0; $i < $ii; $i++) {
		$folderLink .= 'dir[]=' . rawurlencode($outDirs[$i]) . '&';
		$fileLink .= rawurlencode($outDirs[$i]) . '/';
	}

	$ii = count($folders);
	for ($i = 0; $i < $ii; $i++) {
		echo '<a href="?' . $folderLink . 'dir[]=' . rawurlencode($folders[$i]) . '"><img src="http://example.com/data/images/folder.png" />&nbsp;' . $folders[$i] . '</a><br />';
	}

	$ii = count($files);
	for ($i = 0; $i < $ii; $i++) {
		echo '<a href="' . $fileLink . rawurlencode($files[$i]) . '"><img src="http://example.com/data/images/reader.png" />&nbsp;' . $files[$i] . '</a><br />';
	}

}

?>
2

I briefly tried your code and it did not do anything except produce a blank screen?

To make your script more portable:
include your script as a library of functions or classes?
write a stand-alone test menu

Also above each function have a brief:
usage:
parameters:
normal expected results:
exceptions if function fails

.

3

wow! I’ve seen code from others who were new to PHP that was way worse! I’m guessing you already knew another programming language?

Overall the code looks pretty good, well laid out, good to follow.

Some pointers

  • if (isset($_GET[‘dir’])) { … } if (!isset($_GET[‘dir’])) { … }
    can be replaced by if (isset($_GET[‘dir’])) { … } else { … } as the two conditions are mutually exclusive
  • I don’t really see the need to put paths in an array, strings would work just as well, but that’s more a matter of taste then functionality
  • I don’t like the $ii, because it’s a recipe for disaster. One day you’ll find yourself debugging your code for hours, only to find to you used $ii instead of $i or the other way around. I mostly use descriptive variables names like $numDirs. Again, also a matter of taste I suppose :slight_smile:
4

Couple of things that leap out at me…


    $ii = count($folders);
    for ($i = 0; $i < $ii; $i++) {
        echo '<a href="?dir[]=' . rawurlencode($folders[$i]) . '"><img src="http://example.com/data/images/folder.png" />&nbsp;' . $folders[$i] . '</a><br />';
    }

$ii is redundant - you’re not changing the size of the $folders (or $files) array inside the loop, so putting the count($folders) directly into the for statement is sufficient. For that matter, a FOREACH would accomplish this without the use of the extra variables at all!


if (isset($_GET['dir'])) {

This should be the ELSE clause from the previous IF.

Be careful when using file structures that you dont pull . or … (Havent used glob enough to know if it avoids them intentionally)

5

@StarLion:

By putting the count in the for loop, the array would be recounted every iteration, decreasing efficiency.

However, that should all be irrelevant when one takes into account the possible use of foreach.

foreach($folders as $folder){
    //...
}

As you are doing the same things either side of the if() statement, consider some functions which cover your repeat code:


<?php

function getFilesAndFolders($path=''){
    $search = $_SERVER['DOCUMENT_ROOT'] . '/data/docs/example/' . $path;
    return array(
        'folders' => str_replace($search, '', glob($search . '*', GLOB_ONLYDIR)),
        'files' => str_replace($search, '', glob($search . '*.[pP][dD][fF]'))
    );
}

function outputFolderLink($Folder, $Base = ''){
	echo '<a href="?' . $Base . 'dir[]=' . rawurlencode($Folder) . '"><img src="http://example.com/data/images/folder.png" />&nbsp;' . $Folder . '</a><br />';
}

function outputFileLink($File, $Base = ''){
    echo '<a href="' . $Base . rawurlencode($File) . '"><img src="http://example.com/data/images/reader.png" />&nbsp;' . $File . '</a><br />';
}

if (!isset($_GET['dir'])) { // code to execute if no directory has been chosen

    $FilesAndFolders = getFilesAndFolders();

    foreach($FilesAndFolders['folders'] as $folder){
	    outputFolderLink($folder);
    }

    foreach($FilesAndFolders['files'] as $file){
        outputFileLink($file);
    }

}

if (isset($_GET['dir'])) { // code to execute when directory has been chosen
    foreach ($_GET['dir'] as $dir) {
        $path .= strip_tags(rawurldecode($dir)) + '/';
    }

    $FilesAndFolders = getFilesAndFolders();

    $outDirs = explode('/', $path, -1);
    $fileLink = 'http://example.com/data/docs/example/';
    
    foreach($outDirs as $outDir){
        $folderLink .= 'dir[]=' . rawurlencode($outDir) . '&';
        $fileLink .= rawurlencode($outDir) . '/';
    }

    foreach($FilesAndFolders['folders'] as $folder) {
		outputFolderLink($folder, $folderLink);
    }

    foreach($FilesAndFolders['files'] as $file){
        outputFileLink($file, $fileLink);
    }
}
?>
6

Thanks for the tips guys, and no, I’ve never programmed before, PHP was my first.

I’m going to rewrite this again and see if I can clean it up and get my repeat code into functions.

Edit: @John Betong - I haven’t got that far in my book to learn classes yet, I have no knowledge of what OOP is yet either, getting there though.