[ciapug] Peer review of PHP code
Barry Von Ahsen
ciapug@cialug.org
Mon, 12 Apr 2004 16:28:57 -0500
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()); }
> } ?>