[ciapug] Peer review of PHP code
David Champion
ciapug@cialug.org
Mon, 12 Apr 2004 17:04:46 -0500
I would suggest... just for readability:
change -
echo " \n"; echo " \n"; echo " \n"; echo " \n"; echo " \n"; echo " \n"; ...
to -
echo "\n\n\n\n\n\n";
Should give you the same results.
Or you could get fancy and write a function to generate newlines, and
pass it the # of lines you want. :p
-dc
Barry Von Ahsen wrote:
> good:
> -database abstraction (pear?)
> -explicit use of superglobal arrays
> -lots of error and bound checking
>
> bad:
> -non-compliance with the One True Brace Style :) (probably written
> with emacs, too ;)
>
> I personally turn the interpreter on and off as necessary (ie. not
> writing html with php), but that's a personal preference (plus then
> the syntax checker checks the html syntax, not the php). I know this
> was bad performance-wise with ASP2.0/IIS4, but I haven't heard of any
> problems with php.
>
> I'm of the belief that if it works, it's (probably) good enough. My
> code makes perfect sense to me, but I can only imagine what it looks
> like to others (and I have enough co-workers on the list that they'll
> speak up if it's that bad (and I know they will))
>
> -barry
>
>
> Claus wrote:
>
>> Hello
>>
>> I finished up what I call a halfway decent php program but I'm
>> curious what you think about the coding (style as well as function)
>> and would love to hear your comments about it.
>>
>> The program is an administrative interface to upload newsletters
>> (word and or pdf files) to a web site. Functions of the program are
>> to add newsletters, list existing newsletters, replace newsletters,
>> delete newsletters. All functions are implemented within the php
>> file (called newsletters.html) that is attached.
>>
>> I appreciate your feedback as I'm trying to improve my PHP skills.
>> Claus
>>
>> ------------------------------------------------------------------------
>>
>> disconnect(); function outputHtmlHeader() { echo "\n"; echo " \n";
>> echo " \n"; echo " \n"; echo "\n"; echo " \n"; echo "
>>
>>
>> Administration - Newsletters
>>
>> \n"; echo "
>> ------------------------------------------------------------------------
>> \n"; } function outputHtmlFooter() { echo "
>> ------------------------------------------------------------------------
>> \n"; echo " /Last modified: " . date("F d, Y H:i:s", getlastmod()) .
>> "/\n"; echo " \n"; echo "\n"; } function displayNewsletters($db) {
>> echo "
>> \n"; echo " \n"; echo " \n"; echo "
>>
>>
>> Newsletters
>>
>> \n"; echo " \n"; echo "
>> \n"; echo " \n"; echo " \n"; echo " \n"; echo " \n"; echo " *Word
>> Document*\n"; echo " \n"; echo " \n"; echo " *PDF Document*\n";
>> echo " \n"; echo "
>> \n"; $result = $db->query("select year, month, word_file, pdf_file
>> from newsletters order by year desc, month desc"); if
>> (DB::isError($result)) {dbError($result);} while ($resultRow =
>> $result->fetchRow(DB_FETCHMODE_ASSOC)) { echo " \n"; echo " \n"; echo "
>> \n"; echo " \n"; echo " \n"; echo " \n"; echo " \n"; echo "
>>
>> \n"; echo " \n"; echo " \n"; if ($resultRow['word_file'] > ' ') {
>> echo " " . intToTextMonth($resultRow['month']) . " " .
>> $resultRow['year'] . " <\"newsletters/">\n"; } echo " \n"; echo "
>> \n"; if ($resultRow['pdf_file'] > ' ') { echo " " .
>> intToTextMonth($resultRow['month']) . " " . $resultRow['year'] . "
>> <\"newsletters/">\n"; } echo " \n"; echo "
>> \n"; } echo "
>>
>> \n"; } function displayAddNewsletter() { echo "
>> \n"; echo " \n"; echo " \n"; echo "
>> \n"; echo " \n"; echo " \n"; echo "
>>
>>
>> Add Newsletter
>>
>> \n"; echo " \n"; echo "
>> \n"; echo " \n"; echo " \n"; echo " *Word Document*\n"; echo " \n";
>> echo " \n"; echo "
>> \n"; echo " \n"; echo "
>> \n"; echo " \n"; echo " \n"; echo " *PDF Document*\n"; echo " \n";
>> echo " \n"; echo "
>> \n"; echo " \n"; echo "
>> \n"; echo " \n"; echo " \n"; echo " \n"; echo " \n"; echo " \n"; echo
>> " \n"; echo "
>> \n"; echo "
>>
>> \n"; echo "
>>
>> \n"; } function addNewsletter($db) { $result = $db->query("select '1'
>> from newsletters where year = '" . $_POST['keyYear'] . "' and month =
>> '". $_POST['keyMonth'] . "';"); if (DB::isError($result))
>> {dbError($result);} if ($result->numRows() > 0) { echo "
>> \n"; echo " *Newsletter exists already*
>> \n"; echo " Newsletter for " . intToTextMonth($_POST['keyMonth']) . "
>> " . $_POST['keyYear'] . " was not added\n"; echo "
>>
>> \n"; return; } if ($_FILES['docFile']['size'] == 0 &&
>> $_FILES['pdfFile']['size'] == 0) { echo "
>> \n"; echo " *No newsletter uploaded*
>> \n"; echo " Newsletter for " . intToTextMonth($_POST['keyMonth']) . "
>> " . $_POST['keyYear'] . " was not added\n"; echo "
>>
>> \n"; return; } if ($_FILES['docFile']['size'] > 0) { $uploadfile =
>> "newsletters/" . $_REQUEST['keyYear'] . "-" . $_REQUEST['keyMonth'] .
>> " Newsletter.doc"; if
>> (move_uploaded_file($_FILES['docFile']['tmp_name'], $uploadfile)) {
>> chmod($uploadfile, 0775); } else { echo "
>> \n"; echo " *File upload error (doc)*
>> \n"; echo " Newsletter for " . intToTextMonth($_POST['keyMonth']) . "
>> " . $_POST['keyYear'] . " was not added\n"; echo "
>>
>> \n"; return; } } if ($_FILES['pdfFile']['size'] > 0) { $uploadfile =
>> "newsletters/" . $_REQUEST['keyYear'] . "-" . $_REQUEST['keyMonth'] .
>> " Newsletter.pdf"; if
>> (move_uploaded_file($_FILES['pdfFile']['tmp_name'], $uploadfile)) {
>> chmod($uploadfile, 0775); } else { echo "
>> \n"; echo " *File upload error (pdf)*
>> \n"; echo " Newsletter for " . intToTextMonth($_POST['keyMonth']) . "
>> " . $_POST['keyYear'] . " was not added\n"; echo "
>>
>> \n"; return; } } if ($_FILES['docFile']['size'] > 0) { $wordFilename
>> = $_REQUEST['keyYear'] . "-" . $_REQUEST['keyMonth'] . "
>> Newsletter.doc"; } else { $wordFilename = " "; } if
>> ($_FILES['pdfFile']['size'] > 0) { $pdfFilename =
>> $_REQUEST['keyYear'] . "-" . $_REQUEST['keyMonth'] . "
>> Newsletter.pdf"; } else { $pdfFilename = " "; } $result =
>> $db->query("insert into newsletters values ('" . $_REQUEST['keyYear']
>> . "', '" . $_REQUEST['keyMonth'] . "', '" . $wordFilename . "', '" .
>> $pdfFilename . "')"); if (DB::isError($result)) {
>> if(stristr($result->getUserinfo(),"duplicate key")) { echo "
>> \n"; echo " *Newsletter for this month exists already*
>> \n"; echo " Newsletter for " . intToTextMonth($_POST['keyMonth']) . "
>> " . $_POST['keyYear'] . " was not added\n"; echo "
>>
>> \n"; return; } else { dbError($result); } } } function
>> editNewsletter($db) { $result = $db->query("select word_file,
>> pdf_file from newsletters where year = '" . $_POST['keyYear'] . "'
>> and month = '" . $_POST['keyMonth'] ."'"); if (DB::isError($result))
>> {dbError($result);} if (!$resultRow =
>> $result->fetchRow(DB_FETCHMODE_ASSOC)) { echo "
>> \n"; echo " *Some wacky error happened*
>> \n"; echo " Newsletter for " . intToTextMonth($_POST['keyMonth']) . "
>> " . $_POST['keyYear'] . " does not exist and thus can't be edited\n";
>> echo "
>>
>> \n"; return; } if (isset($_POST['editAction']) &&
>> $_POST['editAction'] == "add/replaceDoc") { editAddReplaceDoc($db,
>> $resultRow); } elseif (isset($_POST['editAction']) &&
>> $_POST['editAction'] == "add/replacePdf") { editAddReplacePdf($db,
>> $resultRow); } elseif (isset($_POST['editAction']) &&
>> $_POST['editAction'] == "deleteDoc") { editDeleteDoc($db,
>> $resultRow); } elseif (isset($_POST['editAction']) &&
>> $_POST['editAction'] == "deletePdf") { editDeletePdf($db,
>> $resultRow); } else { editDisplayForm($db, $resultRow); } } function
>> editDisplayForm($db, $resultRow) { echo "
>> \n"; echo " \n"; echo " \n"; echo "
>>
>>
>> Edit Newsletter for " . intToTextMonth($_POST['keyMonth']) . " " .
>> $_POST['keyYear'] . "
>>
>> \n"; echo " \n"; echo "
>> \n"; echo " \n"; echo " \n"; echo " *Word Document*\n"; echo " \n";
>> echo " \n"; echo "
>> \n"; echo " \n"; echo " \n"; echo " \n"; echo " \n"; echo " \n"; echo
>> " \n"; if ($resultRow['word_file'] > ' ') { echo " \n"; echo "
>>
>> \n"; echo " \n"; echo " \n"; echo "
>> \n"; echo " \n"; echo " \n"; echo " \n"; echo " \n"; echo " \n"; echo "
>>
>> \n"; } else { echo " \n"; echo " \n"; echo " \n"; echo " \n"; }
>> echo " \n"; echo "
>> \n"; echo " \n"; echo " \n"; echo " *PDF Document*\n"; echo " \n";
>> echo " \n"; echo "
>> \n"; echo " \n"; echo " \n"; echo " \n"; echo " \n"; echo " \n"; echo
>> " \n"; if ($resultRow['pdf_file'] > ' ') { echo " \n"; echo "
>>
>> \n"; echo " \n"; echo " \n"; echo "
>> \n"; echo " \n"; echo " \n"; echo " \n"; echo " \n"; echo " \n"; echo "
>>
>> \n"; } else { echo " \n"; echo " \n"; echo " \n"; echo " \n"; }
>> echo " \n"; echo "
>> \n"; echo " \n"; echo " \n"; echo "
>> \n"; echo " \n"; echo "
>>
>> \n"; echo " \n"; echo "
>> \n"; echo "
>>
>> \n"; } function editAddReplaceDoc($db, $resultRow) { if
>> ($_FILES['docFile']['size'] == 0) { echo "
>> \n"; echo " *No new Word newsletter uploaded*
>> \n"; echo " Newsletter for " . intToTextMonth($_POST['keyMonth']) . "
>> " . $_POST['keyYear'] . " was not changed\n"; echo "
>>
>> \n"; editDisplayForm($db, $resultRow); return; } $uploadfile =
>> "newsletters/" . $_REQUEST['keyYear'] . "-" . $_REQUEST['keyMonth'] .
>> " Newsletter.doc"; if
>> (move_uploaded_file($_FILES['docFile']['tmp_name'], $uploadfile)) {
>> chmod($uploadfile, 0775); } else { echo "
>> \n"; echo " *File upload error (doc)*
>> \n"; echo " Word newsletter for " .
>> intToTextMonth($_POST['keyMonth']) . " " . $_POST['keyYear'] . "
>> failed to upload\n"; echo "
>>
>> \n"; editDisplayForm($db, $resultRow); return; } $wordFilename =
>> $_REQUEST['keyYear'] . "-" . $_REQUEST['keyMonth'] . "
>> Newsletter.doc"; $result = $db->query("update newsletters set
>> word_file = '" . $wordFilename . "' where year = '" .
>> $_REQUEST['keyYear'] . "' and month = '" . $_REQUEST['keyMonth'] .
>> "'"); if (DB::isError($result)) {dbError($result);} if
>> ($db->affectedRows() == 0) { echo "
>> \n"; echo " *Some wacky error happened*
>> \n"; echo " Word newsletter for " .
>> intToTextMonth($_POST['keyMonth']) . " " . $_POST['keyYear'] . " was
>> not updated in database\n"; echo "
>>
>> \n"; } $resultRow['word_file'] = $wordFilename; editDisplayForm($db,
>> $resultRow); } function editAddReplacePdf($db, $resultRow) { if
>> ($_FILES['pdfFile']['size'] == 0) { echo "
>> \n"; echo " *No new PDF Newsletter uploaded*
>> \n"; echo " Newsletter for " . intToTextMonth($_POST['keyMonth']) . "
>> " . $_POST['keyYear'] . " was not changed\n"; echo "
>>
>> \n"; editDisplayForm($db, $resultRow); return; } $uploadfile =
>> "newsletters/" . $_REQUEST['keyYear'] . "-" . $_REQUEST['keyMonth'] .
>> " Newsletter.pdf"; if
>> (move_uploaded_file($_FILES['pdfFile']['tmp_name'], $uploadfile)) {
>> chmod($uploadfile, 0775); } else { echo "
>> \n"; echo " *File upload error (pdf)*
>> \n"; echo " PDF newsletter for " . intToTextMonth($_POST['keyMonth'])
>> . " " . $_POST['keyYear'] . " failed to upload\n"; echo "
>>
>> \n"; editDisplayForm($db, $resultRow); return; } $pdfFilename =
>> $_REQUEST['keyYear'] . "-" . $_REQUEST['keyMonth'] . "
>> Newsletter.pdf"; $result = $db->query("update newsletters set
>> pdf_file = '" . $pdfFilename . "' where year = '" .
>> $_REQUEST['keyYear'] . "' and month = '" . $_REQUEST['keyMonth'] .
>> "'"); if (DB::isError($result)) {dbError($result);} if
>> ($db->affectedRows() == 0) { echo "
>> \n"; echo " *Some wacky error happened*
>> \n"; echo " PDF newsletter for " . intToTextMonth($_POST['keyMonth'])
>> . " " . $_POST['keyYear'] . " was not updated in database\n"; echo "
>>
>> \n"; } $resultRow['pdf_file'] = $pdfFilename; editDisplayForm($db,
>> $resultRow); } function editDeleteDoc($db, $resultRow) { if
>> ($resultRow['pdf_file'] > ' ') { $result = $db->query("update
>> newsletters set word_file = ' ' where year = '" . $_POST['keyYear'] .
>> "' and month = '" . $_POST['keyMonth'] ."'"); if
>> (DB::isError($result)) {dbError($result);} if ($db->affectedRows() ==
>> 0) { echo "
>> \n"; echo " *Some wacky error happened*
>> \n"; echo " Newsletter for " . intToTextMonth($_POST['keyMonth']) . "
>> " . $_POST['keyYear'] . " does not exist and thus can't be
>> deleted\n"; echo "
>>
>> \n"; } else { if (!unlink("newsletters/" . $resultRow[word_file])) {
>> echo "
>> \n"; echo " *Some wacky error happened*
>> \n"; echo " Word document of " . intToTextMonth($_POST['keyMonth']) .
>> " " . $_POST['keyYear'] . " newsletter could not be deleted\n"; echo "
>>
>> \n"; } } $resultRow['word_file'] = " "; editDisplayForm($db,
>> $resultRow); } else { $result = $db->query("delete from newsletters
>> where year = '" . $_POST['keyYear'] . "' and month = '" .
>> $_POST['keyMonth'] ."'"); if (DB::isError($result))
>> {dbError($result);} if ($db->affectedRows() == 0) { echo "
>> \n"; echo " *Some wacky error happened*
>> \n"; echo " Newsletter for " . intToTextMonth($_POST['keyMonth']) . "
>> " . $_POST['keyYear'] . " does not exist and thus can't be
>> deleted\n"; echo "
>>
>> \n"; } else { if (!unlink("newsletters/" . $resultRow[word_file])) {
>> echo "
>> \n"; echo " *Some wacky error happened*
>> \n"; echo " Word document of " . intToTextMonth($_POST['keyMonth']) .
>> " " . $_POST['keyYear'] . " newsletter could not be deleted\n"; echo "
>>
>> \n"; } } displayAddNewsletter(); displayNewsletters($db); } }
>> function editDeletePdf($db, $resultRow) { if ($resultRow['word_file']
>> > ' ') { $result = $db->query("update newsletters set pdf_file = ' '
>> where year = '" . $_POST['keyYear'] . "' and month = '" .
>> $_POST['keyMonth'] ."'"); if (DB::isError($result))
>> {dbError($result);} if ($db->affectedRows() == 0) { echo "
>> \n"; echo " *Some wacky error happened*
>> \n"; echo " Newsletter for " . intToTextMonth($_POST['keyMonth']) . "
>> " . $_POST['keyYear'] . " does not exist and thus can't be
>> deleted\n"; echo "
>>
>> \n"; } else { if (!unlink("newsletters/" . $resultRow[pdf_file])) {
>> echo "
>> \n"; echo " *Some wacky error happened*
>> \n"; echo " PDF document of " . intToTextMonth($_POST['keyMonth']) .
>> " " . $_POST['keyYear'] . " newsletter could not be deleted\n"; echo "
>>
>> \n"; } } $resultRow['pdf_file'] = " "; editDisplayForm($db,
>> $resultRow); } else { $result = $db->query("delete from newsletters
>> where year = '" . $_POST['keyYear'] . "' and month = '" .
>> $_POST['keyMonth'] ."'"); if (DB::isError($result))
>> {dbError($result);} if ($db->affectedRows() == 0) { echo "
>> \n"; echo " *Some wacky error happened*
>> \n"; echo " Newsletter for " . intToTextMonth($_POST['keyMonth']) . "
>> " . $_POST['keyYear'] . " does not exist and thus can't be
>> deleted\n"; echo "
>>
>> \n"; } else { if (!unlink("newsletters/" . $resultRow[pdf_file])) {
>> echo "
>> \n"; echo " *Some wacky error happened*
>> \n"; echo " PDF document of " . intToTextMonth($_POST['keyMonth']) .
>> " " . $_POST['keyYear'] . " newsletter could not be deleted\n"; echo "
>>
>> \n"; } } displayAddNewsletter(); displayNewsletters($db); } }
>> function intToTextMonth($month) { switch ($month) { case 1: return
>> "January"; break; case 2: return "February"; break; case 3: return
>> "March"; break; case 4: return "April"; break; case 5: return "May";
>> break; case 6: return "June"; break; case 7: return "July"; break;
>> case 8: return "August"; break; case 9: return "September"; break;
>> case 10: return "October"; break; case 11: return "November"; break;
>> case 12: return "December"; break; default: return "Error:
>> intToTextMonth()"; break; } } function dbError($dbReturn) {
>> if($GLOBALS["debug"]) { die($dbReturn->getDebugInfo()); } else {
>> die($dbReturn->getMessage()); } } ?>
>
>
> _______________________________________________
> ciapug mailing list
> ciapug@cialug.org
> http://cialug.org/mailman/listinfo/ciapug
>