Professional PHP5 (Programmer to Programmer Series)
ISBN: N/A
EAN: N/A
EAN: N/A
Year: 2003
Pages: 182
Pages: 182
Before taking off and completing your new stories, consider the state of your current corpus of code.
Yes, some bits are a ugly, but it's completely functional. Some bits are obviously copied and pasted from other places, but it works. It does, however, come with a great testing suite.
Don't knock yourself for a few warts on a fully tested working product. It happens and it's called "code debt." Just as in real life, when you're in a hurry or circumstances require, you can go beyond your means monetarily by going into debt.
Now that you have a bit of time to clean things up, it's time to work off the debt. It's easy and is best if you follow good development guidelines as you code in order to be as productive as you can without going into debt.
What is this referring to? Take a look at your current working files:
index.php customer-contacts.php contact.phpm test.contact.php widgetsession.phpm test.widgetsession.php templates/customer-contacts.tpl templates/footer.tpl templates/header.tpl templates/login.tpl templates/main.tpl templates/thankyou.tpl
. . .and think: "Are all these files required to be in the root directory?" How about if you keep only client "landing points" in there?
Follow that lead and get rid of those PHP-modules (.phpm) files and tests. These are certainly things that a regular Web browser shouldn't accidentally land on. Move them into the following hierarchy:
index.php customer-contacts.php lib/contact.phpm lib/test.contact.php lib/widgetsession.phpm lib/test.widgetsession.php templates/customer-contacts.tpl templates/footer.tpl templates/header.tpl templates/login.tpl templates/main.tpl templates/thankyou.tpl
This is now much better because your root directory has only two files, and they're both landed upon by the Web browser. You also might consider moving your testing files (test.*) into another separate directory, but the creation of a directory doesn't pay off if it contains only one or two files. Go ahead and do it when you have at least four testing files to drop in.
After you've moved the files, go through and make sure that you update your includes and requires, and ensure that your tests continue to run. Cleaning your house/apartment is slightly less fun, but you end up with the same nice feeling afterward.
There are some simple tasks to keep in mind when developing your code:
Keep it simple.
Don't create things you won't use.
Remove duplication, an idea also known as Once and Only Once.
Remove unused functionality, including classes and functions.
The last item is especially hard to do because writing code is hard, so why throw away "good work''? For the same reason that you prune your plants, getting rid of the dead wood leads to a healthier overall system.
Besides, your code is in a source code repository, right? Use CVS, Subversion, SourceSafe, or whatever you have at hand, but put all your files into your source code repository.
Just keep the coding guidelines in mind when knocking out your simple half-point tasks, such as the Logout button, because the footer.tpl is used everywhere.
<br><br> <a href="index.php?action=logout">Logout</a><br> <hr> For Widget World use only - testing environment. </body> </html>
Now, before you modify the index, look at the copy-and-paste at the top of customer-contacts.php and index.php; both of those files start with:
require_once ("Smarty.class.php"); require_once ("widgetsession.phpm"); $session = new WidgetSession(array ('phptype' => "pgsql", 'hostspec' => "localhost", 'database' => "widgetworld", 'username' => "wuser", 'password' => "foobar")); $session->Impress(); $smarty = new Smarty;
It violates the "Once and Only Once" rule, so remove it just cut it out and put it in another file. For now, put it in lib/common.php.
Also, you really shouldn't have logins and passwords in your Web space, so pull the content out and put it another file that is accessible by PHP but well outside the Web root where Apache can serve it up to the curious and/or malicious.
Remember that because we're dealing with settings, per request, add a setting for the maximum weekly contacts. All this should be placed in a new file ../../../../settings.php, placed inconveniently outside of the Web root so that someone cannot use your own Web server to read the configuration file and consequently gain access to your database.
<?php /* * database setup */ $GLOBALS["db-type"] = "pgsql"; $GLOBALS["db-hostname"] = "localhost"; $GLOBALS["db-username"] = "wuser"; $GLOBALS["db-password"] = "foobar"; $GLOBALS["db-name"] = "widgetworld"; /* * 3rd party environment */ $GLOBALS["smarty-path"] = "/usr/lib/php/smarty/"; /* * system settings */ $GLOBALS["max-weekly-contacts"] = 5; ?>
Your lib/common.php file should contain:
<?php require_once ("../../../../settings.php"); require_once ("./lib/widgetsession.phpm"); require_once ($GLOBALS["smarty-path"].'Smarty.class.php'); $smarty = new Smarty; $session = new WidgetSession(array ('phptype' => $GLOBALS["db-type"], 'hostspec' => $GLOBALS["db-hostname"], 'database' => $GLOBALS["db-name"], 'username' => $GLOBALS["db-username"], 'password' => $GLOBALS["db-password"])); $session->Impress(); ?>
While you're at it, you might as well require a login for every page other than index.php, which is currently the only page utilizing the login feature. Append this to your common.php:
/* * require login */ $scriptname = end(explode("/", $_SERVER["REQUEST_URI"])); if ($scriptname <> "index.php") { if ($session->isLoggedIn() == false) { Header ("Location: index.php"); } }
Now turn your attention to index.php, which, after you've added the logic to handle the logout, now looks like this (yes it handles all cases):
<?php require_once ("lib/common.php"); if (array_key_exists("action", $_REQUEST)) { switch ($_REQUEST["action"]) { case "login": $session->login($_REQUEST["login_name"],$_REQUEST["login_pass"]); if ($session->isLoggedIn()) { $smarty->assign_by_ref("user", $session->getUserObject()); $smarty->display ("main.tpl"); exit; } else { $smarty->assign('error', "Invalid login, try again."); $smarty->display ("login.tpl"); exit; } break; case "logout": $session->logout(); $smarty->display ("login.tpl"); exit; break; default: $smarty->display ("login.tpl"); exit; } } else { if ($session->isLoggedIn() == true) { $smarty->assign_by_ref("user", $session->getUserObject()); $smarty->display ("main.tpl"); exit; } } $smarty->display ("login.tpl"); ?>
When you're trying to log in, you succeed and go to main.tpl or fail and go back to the login.tpl, and when logging out, you want to log out and go to login.tpl. If there is no action assigned, then you check the login status with $session->isLoggedIn() and finally the last line displays login.tpl even though it's a logical impossibility, currently, for your code to ever reach that point in execution. It may be in the future, however, so it's wise to use this as a "catch-all'' in case you ever slip up.
Even though the preceding code technically works, there is something just not quite right about it. What the code needs to do is determine which screen to display, but it is dependent on both the login status and the request variables that represent state. The code ends up looking too verbose, so you need to take a few minutes to see what can be done to make it a bit more readable; in other words, it's time to refactor.
Refactoring is an ongoing process that may at first seem to be a subjective process, but you'll still objectively end up with a better-looking code base because of it.
Now back to the matter at hand. There is a technique for refactoring out similarities by creating similarities. It seems a little weird at first, but if your code smells as though it is a candidate for the removal of duplication, (also known as a "Once and Only Once" refactoring), then this technique is probably worth looking into.
Examine the code and consider how you can logically create similarities with what is already there. For instance, the following continues to be perfectly legal PHP it passes the tests and continues to work on the browser side:
<?php require_once ("lib/common.php"); if (array_key_exists("action", $_REQUEST)) { switch ($_REQUEST["action"]) { case "login": $session->login($_REQUEST["login_name"],$_REQUEST["login_pass"]); if ($session->isLoggedIn()) { $smarty->assign_by_ref("user", $session->getUserObject()); $smarty->display ("main.tpl"); exit; } else { if (array_key_exists("login_name", $_REQUEST)) { $smarty->assign('error', "Invalid login, try again."); } $smarty->display ("login.tpl"); exit; } break; case "logout": $session->logout(); if ($session->isLoggedIn()) { $smarty->assign_by_ref("user", $session->getUserObject()); $smarty->display ("main.tpl"); exit; } else { if (array_key_exists("login_name", $_REQUEST)) { $smarty->assign('error', "Invalid login, try again."); } $smarty->display ("login.tpl"); exit; } exit; break; default: if ($session->isLoggedIn()) { $smarty->assign_by_ref("user", $session->getUserObject()); $smarty->display ("main.tpl"); exit; } else { if (array_key_exists("login_name", $_REQUEST)) { $smarty->assign('error', "Invalid login, try again."); } $smarty->display ("login.tpl"); exit; } exit; } } else { if ($session->isLoggedIn()) { $smarty->assign_by_ref("user", $session->getUserObject()); $smarty->display ("main.tpl"); exit; } else { if (array_key_exists("login_name", $_REQUEST)) { $smarty->assign('error', "Invalid login, try again."); } $smarty->display ("login.tpl"); exit; } } $smarty->display ("login.tpl"); ?> What you just did was intentionally create similarities by adding the highlighted conditional within your current logical structure. It's copy-and-paste programming at its worst, but for a good reason.
Next, copy the similarities verbatim and create a function passing in any local state that is required. This particular refactoring is called "Extract Method" (see Refactoring by Martin Fowler, Addison-Wesley, 1999, p. 110.):
function displaySmartyPage (&$smarty, &$session, $pageToDisplay) { if ($session->isLoggedIn()) { $smarty->assign_by_ref("user", $session->getUserObject()); $smarty->display ($pageToDisplay); exit; } else { if (array_key_exists("login_name", $_REQUEST)) { $smarty->assign('error', "Invalid login, try again."); } $smarty->display ("login.tpl"); exit; } }
Now with the new function displaySmartyPage() at your disposal, what remains with index.php is the following:
if (array_key_exists("action", $_REQUEST)) { switch ($_REQUEST["action"]) { case "login": $session->login($_REQUEST["login_name"],$_REQUEST["login_pass"]); displaySmartyPage ($smarty, $session, "main.tpl"); break; case "logout": $session->logout(); displaySmartyPage ($smarty, $session, "main.tpl"); break; default: displaySmartyPage ($smarty, $session, "main.tpl"); } } else { displaySmartyPage ($smarty, $session, "main.tpl"); } displaySmartyPage ($smarty, $session, "main.tpl"); The best part is that now that the code is clearer to read, it's obvious that your logic can be refactored to:
if (array_key_exists("action", $_REQUEST)) { switch ($_REQUEST["action"]) { case "login": $session->login($_REQUEST["login_name"],$_REQUEST["login_pass"]); break; case "logout": $session->logout(); break; } } displaySmartyPage ($smarty, $session, "main.tpl");
Because the displaySmartyPage() function is being called only once, you probably can now see why you refactored out the code into the displaySmartyPage() function.
Now that the displaySmartyPage() function code is unnecessary, you reverse your previous refactoring of "Extract Method" and refactor it back using the "Inline Method" (Refactoring,p.117) technique. It's exactly the opposite:
<?php require_once ("lib/common.php"); if (array_key_exists("action", $_REQUEST)) { switch ($_REQUEST["action"]) { case "login": $session->login($_REQUEST["login_name"],$_REQUEST["login_pass"]); break; case "logout": $session->logout(); break; } } if ($session->isLoggedIn()) { $smarty->assign_by_ref("user", $session->getUserObject()); $smarty->display ("main.tpl"); } else { if (array_key_exists("login_name", $_REQUEST)) { $smarty->assign('error', "Invalid login, try again."); } $smarty->display ("login.tpl"); } ?>
With just a little bit of effort you significantly reduced the complexity, removed extraneous Smarty calls, and reduced the total line count by more than one third.
As you've seen, reducing complexity can lead to some surprising savings, and the code is easier to read and understand to boot.
Just remember to try to stay out of code debt by exercising the "Once and Only Once" rule. If given a choice, keep the solution simple and don't be afraid to remove unneeded functionality.