Help critique my code

[zulater]

I'm somewhat new to coding php but had a project at work that php/mysql would work great for and would keep us from spending money on outside software.

It works and does so well but I'm sure there is a much smoother/sleeker/more efficient way to accomplish the same goal.  I think my comments in the code itself are decent enough to follow but if you have questions just ask.  I really am just trying to learn from what I did and how I could do it better because we have a few more things we may want to go this route with as well.

Basically we were wanting to keep inventory of data tapes that we periodically write out to in order to keep free space on the system disks and have a backup point to return to.  We wanted to be able to have a history of the tapes as well so we could see if we ever wrote over something we possibly needed (oops).

Anyways here it is. 

Thank you in advance.

<?php
require_once("config.php");
global $prefix, $db, $admin_file;
$conn = mysql_connect("$dbhost","$dbuname","$dbpass");
if (!$conn) die ("Could not connect MySQL");
mysql_select_db($dbname,$conn) or die ("Could not open database");


//Pulling in information passed from search.php

$tapenum = $_POST['tapenum'];
$client = $_POST['client'];
$system = $_POST['system'];
$area =$_POST['area'];
$lineid = $_POST['lineid'];
$flow = $_POST['flow'];
$dataset = $_POST['dataset'];
$comments = $_POST['comments'];
$current = $_POST['current'];

//Setuping up links to move around

echo '<a href="index.php">HOME</a>&nbsp&nbsp&nbsp';
echo '<a href="insert.php">INPUT</a>&nbsp&nbsp&nbsp';
echo '<a href="search.php">SEARCH</a>&nbsp&nbsp&nbsp';
echo '<a href="viewdb.php">VIEW ALL</a>&nbsp&nbsp&nbsp';
echo '<a href="viewcurrentdb.php">VIEW CURRENT</a><br><br>';

//Initalizing variables
$datetext = "";
$tapenumtext = "";
$clienttext = "";
$systemtext = "";
$areatext = "";
$lineidtext = "";
$flowtext = "";
$commentstext = "";
$starttext = "WHERE ";

//These next if statements are used to setup the mysql query
//and are designed to be all inclusive so that the mysql querys
//later on will never have to be edited but can be a string of 
//variables.  It can probably be done much more efficient but
//it works so I'm leaving it alone for now.

if ($tapenum!="") //If the user entered a value do the following
{
  $tapenumtext=" WHERE tapenum LIKE '$tapenum%' "; //since tapenum is the first variable we will attatch the WHERE clause here
  $starttext = ""; //wiping out the startint text that held the WHERE clause since the user entered a tape value.
}
elseif ($client=="" && $system=="" && $area=="" && $lineid=="" && $flow=="" && $dataset=="" && $comments=="") //if everything is blank do this
{
  $starttext = ""; //wiping out the start text WHERE clause because the user entered nothing.  This will return everything in the database now as if the user clicked view database
}
else {} //if none of the conditions are met then it will do nothing

//if-then-else statement for the following variables for the mysql query
if ($client!="" && $tapenum=="") { //if the user entered a variable for client but did not enter one for tapenum do this
$clienttext="client LIKE '$client%'"; //The starttext contains the WHERE clause so we only need the comparitave statements
} elseif ($client!="") //if the user did enter a client name and they also entered a tape number do this
{$clienttext="AND client LIKE '$client%'";} //since the tapenum was entered it contains the WHERE clause, we need to attatch the AND so that we pull the entries that contain both the entered tapenumber and the entered client
else {}

//the other if-then-else statements follow the same format but I went ahead and condensed them to save vertical space
if ($system!="" && $tapenum=="" && $client=="") {
$systemtext=" system LIKE '$system%' ";
} elseif ($system!=="")
{$systemtext=" AND system LIKE '$system%' ";}else{}

if ($area!="" && $tapenum=="" && $client=="" && $system=="") {
$areatext="area LIKE '$area%' ";
} elseif ($area!=""){$areatext="AND area LIKE '$area%' ";}
else{}

if ($lineid!="" && $tapenum=="" && $client=="" && $system=="" && $area=="") {
$lineidtext="lineid LIKE '$lineid%' ";
} elseif ($lineid!=""){$lineidtext="AND lineid LIKE '$lineid%' ";}
else{}

if ($flow!="" && $tapenum=="" && $client=="" && $system=="" && $area=="" && $lineid=="") {
$flowtext=" flow LIKE '$flow%' ";
} elseif ($flow!=""){ $flowtext="AND flow LIKE '$flow%' ";}
else{}

if ($dataset!="" && $tapenum=="" && $client=="" && $system=="" && $area=="" && $lineid=="" && $flow=="") {
$datasettext="dataset LIKE '$dataset%' ";
} elseif ($dataset!=""){$datasettext="AND dataset LIKE '$dataset%' ";}
else{}

if ($comments!="" && $tapenum=="" && $client=="" && $system=="" && $area=="" && $lineid=="" && $flow=="" && $dataset=="") {
$commentstext="comments LIKE '%$comments%' ";
} elseif ($comments!=""){ $commentstext="AND comments LIKE '%$comments%' ";}
else{}
//end of the if-then-else statements

//if-else dependant upon the user wanting to see current tapes or all of the tapes meeting their search criteria

if ($current=='on') {//If the user wanted to only see the current tapes in use and not their complete history do this

//query to clear out temporary tables if they exist
$query = "DROP TABLE IF EXISTS `nuke_tapetemp`";
mysql_query($query);
$query = "DROP TABLE IF EXISTS `nuke_tapetemp2`";
mysql_query($query);

//creating a temporary table in descending ID order so that the group by will work on the last entered data for a unique tape number
$query = "CREATE TABLE `nuke_tapetemp` SELECT * FROM `nuke_tapelog` ORDER BY `id` DESC";
mysql_query($query);

//creating a temporary table grouping multiple tape numbers together using the most recent value
$query = "CREATE TABLE `nuke_tapetemp2` SELECT * FROM `nuke_tapetemp` GROUP BY tapenum";
mysql_query($query);

//actual search query based upon user entered information
$query = "SELECT * FROM `nuke_tapetemp2` $starttext $tapenumtext $clienttext $systemtext $areatext $lineidtext $flowtext $datasettext $commentstext";
$result = mysql_query($query);
if (!$result) die ("No result from query 2 ");

//header row for display
echo "<table border=\"1\" cellpadding=\"3\" cellspacing=\"0\" border=\"1\" width=\"100%\">";
echo "<td align=\"left\"><strong>Tape Number</td><td align=\"left\"><strong>Date</td><td align=\"left\"><strong>Client</td><td align=\"left\"><strong>System</td><td align=\"left\"><strong>Area</td><td align=\"left\"><strong>Line ID</td><td align=\"left\"><strong>Flow</td><td align=\"left\"><strong>Dataset</td><td align=\"left\"><strong>Notes</td><tr>";

//value rows for display
while($row = mysql_fetch_array($result)) {
echo "<tr align=\"left\" cellpadding=\"3\" cellspacing=\"0\" border=\"1\"> <td>". $row[tapenum] . "</td><td>" . $row[date] . "</td><td>" . $row[client] . "</td><td>" . $row[system] . "</td><td>" . $row[area] . "</td><td>" . $row[lineid] ."</td><td>" . $row[flow] ."</td><td>" . $row[dataset] ."</td><td>" . $row[comments] ."</td></tr>";

//removing temporary tables
$query = "DROP TABLE `nuke_tapetemp`";
mysql_query($query);
$query = "DROP TABLE `nuke_tapetemp2`";
mysql_query($query);
}

echo "</table>";


} else {//if the user wants to see the complete history of the tapes used do this

//no temporary tables needed because we just want to grab anything that matches the user entered values wether it's current or in past history
$query = "SELECT * FROM `nuke_tapelog` $starttext $tapenumtext $clienttext $systemtext $areatext $lineidtext $flowtext $datasettext $commentstext";
$result = mysql_query($query);
if (!$result) die ("No result from query");

//header row for display
echo "<table border=\"1\" cellpadding=\"3\" cellspacing=\"0\" border=\"1\" width=\"100%\">";
echo "<td align=\"left\"><strong>Tape Number</td><td align=\"left\"><strong>Date</td><td align=\"left\"><strong>Client</td><td align=\"left\"><strong>System</td><td align=\"left\"><strong>Area</td><td align=\"left\"><strong>Line ID</td><td align=\"left\"><strong>Flow</td><td align=\"left\"><strong>Dataset</td><td align=\"left\"><strong>Notes</td><tr>";

//value rows for display
while($row = mysql_fetch_array($result)) {
echo "<tr align=\"left\" cellpadding=\"3\" cellspacing=\"0\" border=\"1\"> <td>". $row[tapenum] . "</td><td>" . $row[date] . "</td><td>" . $row[client] . "</td><td>" . $row[system] . "</td><td>" . $row[area] . "</td><td>" . $row[lineid] ."</td><td>" . $row[flow] ."</td><td>" . $row[dataset] ."</td><td>" . $row[comments] ."</td></tr>";
}

echo "</table>";
}

?>

[mikefrederick]

few suggestions

$tapenum = $_POST['tapenum'];
$client = $_POST['client'];
$system = $_POST['system'];
$area =$_POST['area'];
$lineid = $_POST['lineid'];
$flow = $_POST['flow'];
$dataset = $_POST['dataset'];
$comments = $_POST['comments'];
$current = $_POST['current'];

to

extract($_POST); //neither way is very secure, you should be using mysql_real_escape_string

echo '<a href="index.php">HOME</a>&nbsp&nbsp&nbsp';
echo '<a href="insert.php">INPUT</a>&nbsp&nbsp&nbsp';
echo '<a href="search.php">SEARCH</a>&nbsp&nbsp&nbsp';
echo '<a href="viewdb.php">VIEW ALL</a>&nbsp&nbsp&nbsp';
echo '<a href="viewcurrentdb.php">VIEW CURRENT</a><br><br>';

-->no need to break up these lines

 

$starttext $tapenumtext $clienttext $systemtext $areatext $lineidtext $flowtext $datasettext $commentstext

->you could compress these all to one variable
instead of $clienttext="client LIKE '$client%'"; 
use $tapenumtext.="client LIKE '$client%'";

[zulater]

Thanks!

I'll have to read up on mysql_real_escape_string but browsing the manual it seems what I have currently is not that bad since there is no password to work around and the users are smart but it's good to get into a better habit.

 

I only broke up the echo 'links' to see it easier.  Not necessary but a preference.  Looking to put them into tables anyway.

 

The last critique is intriguing.  If I'm seeing it right the .= will add to an existing string?  So I could just setup $query and then after each if statement just do $sqlstatement.=$tapenumtext $sqlstatement.=$clienttext etc.  That's neat.