Code critique if possible

[abda53]

I am looking to see some critique on this script, but also to see if you would have a better way to do it.

My client wanted a website where it pulls images from a directory, displays it, and has 1 - 10 image link at the bottom (or 11-20, etc).. and using back and next buttons.


gallery.php

$folder=$_GET['f'];
if ($id==""){echo '<img src="gallery/',$folder,'/1.jpg" height="400">';}
else {echo '<img src="gallery/',$folder,'/',$id,'.jpg" height="400">';}

require "gallery_function.php";




gallery_function.php

$dir_path = "/path/to/gallery/$folder/"; // set the path

$count = count(glob($dir_path . "*")); // count the total images
$i=1; // starting image to show
if (isset($_GET['id'])){$id=$_GET['id']; $nextid=$id+1;}else{$id=1;} // gets the next ID
$max=10; // max amount of links to show
$start=10; // the starting of numbers to show (1-10,11-20) starting with the first number
if (isset($_GET['start'])){$start=$_GET['start'];}else{$start="1";} // pulls the start from GET if applicable
$end = $start+9; //gets the ending number
if ($end>$count){$end=$count;} // checks if number is greater than $count
$nextid = $id+1; // shows the next ID on next
$previd = $id-1; // shows the next ID on next
$nextstart = $start+10; // shows the next available start
$prevstart = $start-10; // shows the next available start
$block = range($start,$end); //finds the range of numbers to use

// pull the BACK image and sends back to main page if - or 0
if ($previd!=0 AND $id!=$start){echo '<td><a href="photography_gallery.php?f=',$folder,'&start=',$start,'&id=',$previd,'"><img src="images/back.jpg"></a></td>';}
elseif ($id<=$start AND $prevstart>=1){echo '<td><a href="photography_gallery.php?f=',$folder,'&start=',$prevstart,'&id=',$previd,'"><img src="images/back.jpg"></a></td>';}
elseif ($prevstar<=0) {echo '<td><a href="photography.php"><img src="images/back.jpg"></a></td>';}

//pull the image links.
foreach ($block AS $b)
{
echo '<td><a '; if ($b=="$id"){echo ' id="gal"';} echo 'href="photography_gallery.php?f=',$folder,'&start=',$start,'&id=',$b,'">',$b,'</a></td>';
echo "\r\n";
}

//pull the NEXT image
if ($nextid<$nextstart AND $nextid<=$count){echo '<td><a href="photography_gallery.php?f=',$folder,'&start=',$start,'&id=',$nextid,'"><img src="images/next.jpg"></a></td>';}
elseif ($nextstart<$count AND $nextid<$count){echo '<td><a href="photography_gallery.php?f=',$folder,'&start=',$nextstart,'&id=',$nextstart,'"><img src="images/next.jpg"></a></td>';}
else {echo '<td width="67">&nbsp;</td>';}

[alanzhao]

It is alright for small project.

I would recommend you use a template system to avoid the mix up of HTML and PHP.

[Weedpacket]

The formatting is ghastly, and some of the comments are incorrect (and some of the rest are redundant).

[MarkR]

1. Do all your require()s before you do anything else in each page - this means that you aren't going to get problems where a page "half" executes.
2. Put everything in a function. I mean it. It's the only way of sanely scoping variables.
3. Don't mix presentation and logic (even presentation-oriented logic) if possible. I prefer to put all code which does "stuff" at the top of the page, before I output anything. This also benefits you if you want to set headers, redirect etc.

Mark