this is a long script.. can it be shorter?

[webchef]

HELLO!!!! and how are you?

ok.. so to understand my code here is what you gotta know.. i learned php4 using a book from wrox. i never did finish the book.. but i got real close to the end.. to be completely honest.. i really learned php by trial and error.. just typing some code!...

well anyway.. the problem with this is that im sure there are many faster, shorter, and more effiecient ways to do what my scripts do... and i wanna get feed back on that.. soooo.. i pasted all my code from the latest script i wrote right here.. below..

the script is for a web form that allows a site administrator to edit the rows in a news table. i actually wrote this script to facilitate the migration of the table to another database.. i wanted to move the table row by row so i can proofread each article, edit it, add stuff to it.. ect... i got pretty much done with the code when i realized that i can just replace the migrate part of the script with edit code and use the page in the future... (for clarification purposes i removed the migrate portion and replaced it with edit code. (it still under the same part of the if_else statement)...

so without further ado.. here is the code.. and now YOU tell ME how can i write it better...!

thanks


<?php
//Database Information
$dbhost = "localhost";
$dbuser = "******";
$dbpass = "******";
$dbname = "*****";

// Connect to MySql
$connect = mysql_connect($dbhost, $dbuser, $dbpass)
or die("Unable to connect to MySql");

// Connect to Database
$db = mysql_select_db($dbname, $connect)
or die("Unable to connect to Database");

// Get all articles into an array for easier article selection and counting
$arraySQL = "Select auto_id FROM loc_news";
$arrayResult = mysql_query($arraySQL)
or die("Unable to perform SQL Query");

$noOfArticles = mysql_num_rows($arrayResult); // To count the number of articles in the table (and array)

$arrayCounter = 0;
while ($arrayRow = mysql_fetch_array($arrayResult)) {
$articleID = $arrayRow["auto_id"];

$arrayCounter++;
$article[$arrayCounter] = $articleID;
}

//Give information about the table
echo "
<b>
There are currently $noOfArticles articles in the <i>Local News</i> table
</b>
<br>
";

//If Else statements to handle article display
if ($goto) { //If a specific article is chosen, the article is checked for existence and then shown
$indexArticle = array_search($articleChoice, $article);

$gotoSQL = "SELECT * FROM loc_news WHERE auto_id = '$articleChoice'";
$gotoResult = mysql_query($gotoSQL)
or die ("Unable to retrieve Article information");

//Make sure the article called exists and act accordingly
$exist = mysql_num_rows($gotoResult);
if ($exist != 1) {
echo "<b>-Sorry, article $articleChoice does not exist. Article 1 is displayed instead:</b>";
$gotoSQL = "SELECT * FROM loc_news WHERE auto_id = '1'";
$gotoResult = mysql_query($gotoSQL)
or die ("Unable to retrieve Article information");
}
else {
echo "<b>-Article $articleChoice was chosen and is displayed [<i>Array Index: $indexArticle</i>]:</b>";
}

while ($gotoRow = mysql_fetch_array($gotoResult)) {
$auto_id = $gotoRow["auto_id"];
$year = $gotoRow["year"];
$month = $gotoRow["month"];
$day = $gotoRow["day"];
$headline = $gotoRow["headline"];
$summary = $gotoRow["summary"];
$courtesy_of = $gotoRow["courtesy_of"];
$body = $gotoRow["body"];
}
}
else if ($prev) {
if ($current == 1) {
echo "<b>-Sorry, there are no previous articles. Article 1 is displayed:</b>";

$articleChoice = 1;
$prevSQL = "SELECT * FROM loc_news WHERE auto_id = '$articleChoice'";
$prevResult = mysql_query($prevSQL)
or die ("Unable to retrieve Article information");

while ($prevRow = mysql_fetch_array($prevResult)) {
$auto_id = $prevRow["auto_id"];
$year = $prevRow["year"];
$month = $prevRow["month"];
$day = $prevRow["day"];
$headline = $prevRow["headline"];
$summary = $prevRow["summary"];
$courtesy_of = $prevRow["courtesy_of"];
$body = $prevRow["body"];
}
}
else {
$indexArticle = $current-1;
$articleChoice = $article[$indexArticle];

echo "<b>-Article $articleChoice was chosen and is displayed [<i>Array Index: $indexArticle</i>]:</b>";

$prevSQL = "SELECT * FROM loc_news WHERE auto_id = '$articleChoice'";
$prevResult = mysql_query($prevSQL)
or die ("Unable to retrieve Article information");

while ($prevRow = mysql_fetch_array($prevResult)) {
$auto_id = $prevRow["auto_id"];
$year = $prevRow["year"];
$month = $prevRow["month"];
$day = $prevRow["day"];
$headline = $prevRow["headline"];
$summary = $prevRow["summary"];
$courtesy_of = $prevRow["courtesy_of"];
$body = $prevRow["body"];
}
}
}
else if ($next) {
if ($current == $noOfArticles) {
echo "<b>-Sorry, there are no more articles. Article $article[$noOfArticles] is displayed:</b>";

$articleChoice = $article[$noOfArticles];
$nextSQL = "SELECT * FROM loc_news WHERE auto_id = '$articleChoice'";
$nextResult = mysql_query($prevSQL)
or die ("Unable to retrieve Article information");

while ($nextRow = mysql_fetch_array($nextResult)) {
$auto_id = $nextRow["auto_id"];
$year = $nextRow["year"];
$month = $nextRow["month"];
$day = $nextRow["day"];
$headline = $nextRow["headline"];
$summary = $nextRow["summary"];
$courtesy_of = $nextRow["courtesy_of"];
$body = $nextRow["body"];
}
}
else {
$indexArticle = $current+1;
$articleChoice = $article[$indexArticle];

echo "<b>-Article $articleChoice was chosen and is displayed [<i>Array Index: $indexArticle</i>]:</b>";

$nextSQL = "SELECT * FROM loc_news WHERE auto_id = '$articleChoice'";
$nextResult = mysql_query($nextSQL)
or die ("Unable to retrieve Article information");

while ($nextRow = mysql_fetch_array($nextResult)) {
$auto_id = $nextRow["auto_id"];
$year = $nextRow["year"];
$month = $nextRow["month"];
$day = $nextRow["day"];
$headline = $nextRow["headline"];
$summary = $nextRow["summary"];
$courtesy_of = $nextRow["courtesy_of"];
$body = $nextRow["body"];
}
}
}
else if ($edit) {
// Take submitted information and edit the respected row in the table with the new information
$whereClause = $article[$indexArticle];
$tableSQL = "UPDATE loc_news SET year = '$year', month = '$month', day = '$day', courtesy_of = '$courtesy_of', subject = '$subject', headline = '$headline', summary = '$summary', body = '$body' WHERE auto_id = '$whereClause'";
$tableResult = mysql_query($tableSQL)
or die("Unable to complete table update");

// Get next article
$indexArticle = $indexArticle+1;
$articleChoice = $article[$indexArticle];

// Check to see if there are any more articles and act accordingly
if ($indexArticle <= $noOfArticles) {
echo "<b>-Article $articleChoice was successfully edited. The next article is displayed:</b>";
}
else {
echo "<b>-Article $articleChoice was successfully edited. There are no more articles in the table. The first article is displayed:</b>";
$articleChoice = 1;
}

$editSQL = "SELECT * FROM loc_news WHERE auto_id = '$articleChoice'";
$editResult = mysql_query($editSQL)
or die ("Unable to retrieve Article information");

while ($editRow = mysql_fetch_array($editResult)) {
$auto_id = $editRow["auto_id"];
$year = $editRow["year"];
$month = $editRow["month"];
$day = $editRow["day"];
$headline = $editRow["headline"];
$summary = $editRow["summary"];
$courtesy_of = $editRow["courtesy_of"];
$body = $editRow["body"];
}
}
else {
$indexArticle = 1;
echo "<b>-No article is chosen, the first article in the table is displayed:</b>";

$noneSQL = "SELECT * FROM loc_news WHERE auto_id = '$indexArticle'";
$noneResult = mysql_query($noneSQL)
or die ("Unable to retrieve Article information");

while ($noneRow = mysql_fetch_array($noneResult)) {
$auto_id = $noneRow["auto_id"];
$year = $noneRow["year"];
$month = $noneRow["month"];
$day = $noneRow["day"];
$headline = $noneRow["headline"];
$summary = $noneRow["summary"];
$courtesy_of = $noneRow["courtesy_of"];
$body = $noneRow["body"];
}
}
?>

[webchef]

The form is where all the 'fields' go.. obviously.. so there is a text box or so for every feild.. and radio buttons for the subject field (which is on the new not yet made table) but anyway..

there is a box with a go button called goto.. this is for when a specific article is wanted. the user can input the article id and press go and the script will take him there..

there is also a submit button called 'edit'.

and there are also two links (images with href on them)... the links are for next article and previous article.. this way you can go forward or back without havign to submit or manually choose the article.. the link looks like this:

<a href="page.php?current=<?php echo "$indexArticle"; ?>&prev=1">

[Weedpacket]

Actually I quite like it (trial and error, huh? Well, that's how I learned....) I was going to write something and I actually found that this was more subtle than it looked at first. There is some redundancy that is asking to be moved to a function ("If it's worth doing twice it's worth writing a function for.") but like I said....

Rather than copy everything into a whole bunch of variables, just return the result into the same array and use the array itself. Also, if there is exactly one row returned, you don't need a while loop. In other words, the blocks


while ($gotoRow = mysql_fetch_array($gotoResult)) {
$auto_id = $gotoRow["auto_id"];
$year = $gotoRow["year"];
$month = $gotoRow["month"];
$day = $gotoRow["day"];
$headline = $gotoRow["headline"];
$summary = $gotoRow["summary"];
$courtesy_of = $gotoRow["courtesy_of"];
$body = $gotoRow["body"];
}

could just be replaced by
$article_details = mysql_fetch_array($gotoResult);using "$article_details" each time (since I take it you only ever have the one article at a time). And wherever it is that you use $body, $headline, etc., use $article_details['body'], $article_details['headline'] there instead.

[Weedpacket]

Originally posted by webchef
<a href="page.php?current=<?php echo "$indexArticle"; ?>&prev=1"> If you're echoing a simple variable, you don't need to put it in quotes. PHP will turn it into a string automatically because that's what echo wants.

[Moonglobe]

Originally posted by Weedpacket

while ($gotoRow = mysql_fetch_array($gotoResult)) {
$auto_id = $gotoRow["auto_id"];
$year = $gotoRow["year"];
$month = $gotoRow["month"];
$day = $gotoRow["day"];
$headline = $gotoRow["headline"];
$summary = $gotoRow["summary"];
$courtesy_of = $gotoRow["courtesy_of"];
$body = $gotoRow["body"];
}

could just be replaced by
$article_details = mysql_fetch_array($gotoResult);using "$article_details" each time (since I take it you only ever have the one article at a time). And wherever it is that you use $body, $headline, etc., use $article_details['body'], $article_details['headline'] there instead.
or just use list($auto_id, $year, $month, $day, $headline, $summary, $couresy_of, $body) = mysql_fetch_array($gotoResult);
this requires you to have the columns in order in the database. if they aren't, just flip it around a bit. ;)

hth
moon

[BuzzLY]

well... if it's really that important to put those columns into variables, there is an easier way (that doesn't require the columns to be in the right order).

As Weed said, use $article_details = mysql_fetch_assoc($gotoResult), then do this:
foreach ($article_details as $key=>$value)
{
$$key = $value;
}

[drawmack]

I don't have time to optimize this code right now, I'll do that a little later.

For now just three comments on optimizing code:
1) When optimizing code analyze every loop and if/switch statement. If you minimize the number of these your are on the right path.

2) Do not nest if, switch or loops within each other if at all possible it makes the code difficult to maintain.

3) If you repeat more then two lines of code it need to be made into a function.

I'll do a thorough analysis of this specific code later.

[Merve]

For your array elements, use single quotes, as in $foo['bar']

But that's damn good code...

[webchef]

hahahaha..

thanks for the response guys.. im taking everyting you've said into consideration.. im actually changing the code now.. taking the while loops out and placing the row (it is one row after all) into an array (the list thing was cool too but an array looks a little better.. same thing though)... im also looking into the functions. i've gone back to the wrox book and im trying to figure out how to get most of the article retrieval code into a function...

im also looking forward to that thorough analysis by drawmack.. but i disagree.. i think as long as you write the nested if, switch, and loops clearly the code will be just as easy to manage. .. to each his own... still looking foward to the analysis.

thanks for the critique gentlemen.. waiting for more.. hehehe

Mike

[webchef]

ok right.. so i tried to optimise my code.. and i think i screwed everything up!!!!

i don't know.. the function don't work.. perhaps you can tell me what im doing wrong.. this is the first time im writing a function..

here's the code.. thanks for the help


<?php
//Database Information
$dbhost = "localhost";
$dbuser = "administrator";
$dbpass = "admin";
$dbname1 = "f1bahrain";
$dbname2 = "f1b";

// Connect to MySql
$connect = mysql_connect($dbhost, $dbuser, $dbpass)
or die("Unable to connect to MySql:<br>" .mysql_errno(). " : " .mysql_error());

// Connect to Database F1bahrain
$db = mysql_select_db($dbname1, $connect)
or die("Unable to connect to Database:<br>" .mysql_errno(). " : " .mysql_error());

// Get all articles into an array for easier article selection and counting
$arraySQL = "Select auto_id FROM loc_news";
$arrayResult = mysql_query($arraySQL)
or die("Unable to complete query:<br>" .mysql_errno(). " : " .mysql_error());

// Count the number of articles in the table and array
$noOfArticles = mysql_num_rows($arrayResult);

// Set article ID numbers into an array, starting from one
$arrayCounter = 0;
while ($arrayRow = mysql_fetch_array($arrayResult)) {
$articleID = $arrayRow["auto_id"];
$arrayCounter++;
$article[$arrayCounter] = $articleID;
}

// Print number of articles available based according to the article array
echo "<b><font color=#006699>There are currently $noOfArticles articles in the <i>Local News</i> table</font></b><br>";

// Create function to handle the retrieval of the article to be displayed
function callArticle ($articleChoice) {
// Run query to retrieve article
$sql = "SELECT * FROM loc_news WHERE auto_id = '$articleChoice'";
$result = mysql_query($sql)
or die ("Unable to complete query:<br>" .mysql_errno(). " : " .mysql_error());

// Check the existence of the article
if (!$exist = mysql_num_rows($result)) {
// Run query to retrieve first article
$sql = "SELECT * FROM loc_news WHERE auto_id = '1'";
$result = mysql_query($sql)
or die ("Unable to complete query:<br>" .mysql_errno(). " : " .mysql_error());

$articleDetails = mysql_fetch_array($result);
return $articleDetails;
}
else {
$articleDetails = mysql_fetch_array($result);
return $aticleDetails;
}
}

/* Set variables for array Index and article Id.
This is to make sure I have a value for each,
knowing that I will always have one or the other.*/
if (!$articleIndex) {
$articleIndex = array_search($articleChoice, $article);
}
else {
$articleChoice = $article[$articleIndex];
}

// Decide what action was taken and how to respond
if ($goto) {
// Run the function and break up the array
callArticle($articleChoice);
// Find out if the article existed and print out what action was taken
switch ($exist) {
case '0':
echo "<b>-Sorry, article $articleChoice does not exist. Article 1 is displayed instead:</b>";
break;
default:
echo "<b>-Article $articleChoice was chosen and is displayed [<i>Array Index: $indexArticle</i>]:</b>";
break;
}
}
else {
echo "<b>-No article is chosen, the first article in the table is displayed:</b>";
// Run the function and break up the array
callArticle($articleChoice);
}
?>


there are a couple of else if missing from the last if else structure but right now this is just to get the thing working..

thanks again.

[BuzzLY]

return $aticleDetails
When coding, you have too be VERY carefull about speling. The PHP parser is not as forgiving as the human eye.

Also, your function is returning an array, right? Well... when you call the function, you aren't doing anything with it. Here is how you use a function:

function addTwo($foo) {
$bar = $foo + 2;
return $bar;
}

echo addTwo(1);
// echos "3" to the screen
$four = addTwo(2);
// puts the result of the function into the variable.

Make sense? If you don't want to return a value (and you should always return a value, even if it's just "true" to show that it ran), then simply perform your echo or whatever within the function.

[Ortega]

Originally posted by Moonglobe
or just use list($auto_id, $year, $month, $day, $headline, $summary, $couresy_of, $body) = mysql_fetch_array($gotoResult);
this requires you to have the columns in order in the database. if they aren't, just flip it around a bit. ;)

hth
moon

or you could use
extract(mysql_fetch_array($gotoResult, MYSQL_ASSOC));

[Moonglobe]

Originally posted by Ortega
or you could use
extract(mysql_fetch_array($gotoResult, MYSQL_ASSOC)); ahah omg why didn't i think of that.... i use it often enough lol :p

[dnirvine]

If you want you could probably cut the code down by putting //Database Information
$dbhost = "localhost";
$dbuser = "******";
$dbpass = "******";
$dbname = "*****";

// Connect to MySql
$connect = mysql_connect($dbhost, $dbuser, $dbpass)
or die("Unable to connect to MySql");

// Connect to Database
$db = mysql_select_db($dbname, $connect)
or die("Unable to connect to Database");
in a seperate file such as "db.php" and including it into your script using <?php include ('db.php'); ?>.

[Weedpacket]

Originally posted by dnirvine
If you want you could probably cut the code down by putting ... in a seperate file such as "db.php" and including it into your script.... And as an added security measure, db.php could be put outside the web document tree and would still be inaccessible to anyone even if they'd managed to get far enough into your system to read the source code of your web documents (they'd have to get all the way into the OS's own filesystem to get to db.php).

[babygodzilla]

Originally posted by dnirvine
If you want you could probably cut the code down by putting //Database Information
$dbhost = "localhost";
$dbuser = "******";
...
in a seperate file such as "db.php" and including it into your script using <?php include ('db.php'); ?>.

i was about to type that, good thing i scrolled all the way down :)

this may not help too much but i like to separate my functions from the main code. i usually have a separate file for the functions and include it in the script. if i only have like 1 or 2 functions that's only used locally in one file, i'll at least put it in a separate section, at the top before the main code, or at the bottom, separated by lines of comments like

/************************/
or something like that. i think it helps readability.

just my $.02