Page 1 of 1

Making this chunk of code easier to maintain

Posted: Sun May 14, 2006 9:33 pm
by Ambush Commander
How would I, with the last amount of work, make this page easier to maintain? Try not to go to abstract, because I understand that ideally we'd have a Front-Controller and Commands and the whole shish-kabob. Baby steps please.

Code: Select all

<?php

header('Content-Type: text/html; charset=utf-8');

$LOCAL   = file_exists('local.txt');
$PAGE    = isset($_GET['page']) ? $_GET['page'] : 'Main_Page';
$WEBPATH = $LOCAL ? '/taijiclub' : '';
$LANG    = isset($_GET['lang']) && ($_GET['lang'] == 'ch') ? 'ch' : 'en';
$DISPLAY_PAGE = true;

// bug 2 - automatic UI recompilation
$RECOMPILE_SKINS = false;
if (isset($_GET['recompile_skins'])) {
    $RECOMPILE_SKINS = true;
    $files_to_skin = array(
        'phpbb' => 'forums/templates/subSilver/overall_header.tpl'
       ,'gallery2' => 'gallery2/themes/matrix/templates/local/theme.tpl'
        );
    function recompile_skins($text, $files) {
        foreach ($files as $application => $file) {
            $contents = file_get_contents($file);
            // make sure dot is set to gobble newlines too with pattern modifier 's'
            $contents = preg_replace('/<!-- taijiclub_links -->.*?(<!-- taijiclub_links -->)/s',
                '<!-- taijiclub_links -->'.$text.'<!-- taijiclub_links -->',
                $contents
                );
            $contents = str_replace(html_class_active(true), '', $contents);
            $contents = str_replace('active-' . $application, 'active', $contents);
            $handle = fopen($file, 'w');
            fwrite($handle, $contents);
            fclose($handle);
        }
    }
}

// config
require('classes/DB.php');
require('classes/KeyGenerator.php');
require('classes/DomainObject.php');
$wgLocaltimezone="America/New_York";
putenv("TZ=$wgLocaltimezone");
$wgLocalTZoffset = date("Z") / 3600;
$wgDBTablePrefix = '';

if ($LOCAL) {
    $wgDB = new DB($credentials);
} else {
    $wgDB = new DB($credentials);
}

$STRINGS = parse_ini_file("locales/$LANG.ini",true);

$REQUEST_TYPES = array(
  'Main_Page',
  'About',
  'News',
  'Events',
  'Digest',
  'Taiji_and_I',
  'Downloads',
  'Registration',
  'Contact'
  );

if (in_array($PAGE, $REQUEST_TYPES)) {
    $PAGENAME = $STRINGS['Menu'][$PAGE];
} else {
    header("Location: $WEBPATH/$LANG/Main_Page");
    exit;
}

$CONTENTS = (string) @file_get_contents('data/' . $LANG . '/' . basename($PAGE) . '.html');

if ($PAGE == 'News' &&
    !empty($_GET['month']) && !empty($_GET['year']) &&
    is_numeric($_GET['month']) && is_numeric($_GET['year']) &&
    strlen($_GET['month']) == 2 && strlen($_GET['year']) == 4 &&
    ((int) $_GET['month']) <= 12 && ((int) $_GET['month']) >= 1) {
    
    $CONTENTS = (string) @file_get_contents('data/' . $LANG . '/' .
      basename($PAGE) . '/' . basename($_GET['year']) . '/' .
      basename($_GET['month']) . '.html');
    if ($CONTENTS) $DISPLAY_PAGE = false;
}

if ($PAGE == 'Digest') {
    $category= !empty($_GET['category'])&& ctype_alnum($_GET['category'])? $_GET['category']: null;
    $topic   = !empty($_GET['topic'])   && ctype_alnum($_GET['topic'])   ? $_GET['topic']   : null;
    $article = !empty($_GET['article']) && ctype_alnum($_GET['article']) ? $_GET['article'] : null;
    $DISPLAY_PAGE = false;
    if ($category !== null && $topic !== null && $article !== null) {
        $CONTENTS = (string) @file_get_contents("data/$LANG/Digest/$category/$topic/$article.html");
    } elseif ($category !== null && $topic !== null) {
        $CONTENTS = (string) @file_get_contents("data/$LANG/Digest/$category/$topic.html");
    } elseif ($category !== null) {
        $CONTENTS = (string) @file_get_contents("data/$LANG/Digest/$category.html");
    } else {
        $DISPLAY_PAGE = true;
    }
}

$scripts = array('Registration');
if (in_array($PAGE, $scripts)) {
    require('classes/scripts/' . $PAGE . '.php');
    $class_name = 'Script_' . $PAGE;
    $script = new $class_name();
    $CONTENTS = $script->execute();
}

if (!$CONTENTS) {
    $CONTENTS = (string) @file_get_contents('data/' . $LANG . '/Error.html');
}

//UTF-8 BOM Check
if (substr($CONTENTS,0,3) == chr(239) . chr(187) . chr(191)) {
    $CONTENTS = substr($CONTENTS, 3, strlen($CONTENTS) - 3);
}

//reparse gallery2 to point to taijiclub.org
if ($LOCAL) {
    $CONTENTS = str_replace('/gallery2/', 'http://www.taijiclub.org/gallery2/', $CONTENTS);
}
//expand macros
$CONTENTS = str_replace('{WEBPATH}', $WEBPATH, $CONTENTS);

function html_class_active($bool) {
    if ($bool) return ' class="active"';
}

?><!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html>
<head>
    <title><?php echo $PAGENAME ?> - Huaxia Taiji Club</title>
    <meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
    <link rel="stylesheet" type="text/css" media="screen, projection" href="<?php echo $WEBPATH; ?>/screen.css" />
    <link rel="stylesheet" type="text/css" media="print" href="<?php echo $WEBPATH; ?>/print.css" />
    <!--[if lt IE 7]><style type="text/css">@import "<?php echo $WEBPATH; ?>/IEFixes/common.css";</style><![endif]-->
    <link rel="icon" href="<?php echo $WEBPATH; ?>/favicon.ico" type="image/x-icon" />
    <link rel="shortcut icon" href="<?php echo $WEBPATH; ?>/favicon.ico" type="image/x-icon" />
</head>
<body>

<div id="translation"><a href="<?php echo $WEBPATH; ?>/<?php
  echo ($LANG == 'en') ? 'ch' : 'en' ?>/<?php echo $PAGE; ?>"><?php
  echo ($LANG == 'en') ? '&#20013;&#25991;' : 'English' ?></a></div>

<div id="heading"><a href="<?php echo $WEBPATH; ?>/en/Main_Page" title="English Main Page">Huaxia Taiji Club</a>
  <a class="heading_ch" href="<?php echo $WEBPATH; ?>/ch/Main_Page" title="&#20013;&#25991;&#20027;&#39029;">&#21326;&#22799;&#22826;&#26497;&#20465;&#20048;&#37096;</a></div>
<ul id="menu"<?php if($LANG == 'ch') {?> class="ch"<?php } ?>>
    <?php
    
    $links = '';
    foreach ($REQUEST_TYPES as $value) {
        $active = html_class_active($value == $PAGE);
        $links .= "<li><a href=\"$WEBPATH/$LANG/$value\"{$active}>{$STRINGS['Menu'][$value]}</a></li>";
    }
    $links .=
    '<li><a href="http://www.taijiclub.org/gallery2/main.php" class="active-gallery2">Gallery</a></li>
     <li><a href="http://www.taijiclub.org/forums/index.php" class="active-phpbb">Forums</a></li>';
    
    echo $links;
    
    if ($RECOMPILE_SKINS) recompile_skins($links, $files_to_skin);
    
    ?>
    
</ul>
<div id="content">
<?php if($DISPLAY_PAGE) { ?><h1 id="title"><?php echo $PAGENAME; ?></h1><?php } ?>
<?php echo $CONTENTS; ?>
</div>
</body>
</html>

Re: Making this chunk of code easier to maintain

Posted: Sun May 14, 2006 10:36 pm
by Christopher
Ambush Commander wrote:How would I, with the last amount of work, make this page easier to maintain?

Is it hard to maintain? It looks fairly clean to me. There is a little spaghetti with the config and some of the response, but I would maintain it pretty easily. I may be considered an OO guy by some, but it is the simplicity of PHP scripts like this one that make is such a great language (elitists be damned).
Ambush Commander wrote:Try not to go to abstract, because I understand that ideally we'd have a Front-Controller and Commands and the whole shish-kabob. Baby steps please.
If you really want baby steps then start splitting it vertically as it is pretty clearly laid out.

- The top is config stuff, but there is also config stuff further on. That could be consolidated and simplified because it seems to be spread out. The local setting could probably be handled with the normal config stuff.

- You have a pre-filter that recompiles. You could move that to an include so you only parse the code when needed.

- Pretty standard front controller code already, but you always do routing logic checks rather than moving the page specific stuff into the individual page classes

- Finally you have the response stuff. Probably hierarchical templates would clean this up. I like a Response class because it can have standard layout defaults that most pages use, but they can be overridden. And because that stuff is not loaded until the end there is little cost for the flexiblity.

Posted: Mon May 15, 2006 10:42 am
by pickle
- Put all you functions together at the bottom or top of the file
- Put all your require() statements at the top.
- Don't jump in & out of PHP - use heredoc's if you've got big chunks of text
- Put all your initialization statements & logic together.

Posted: Mon May 15, 2006 12:36 pm
by Chris Corbyn
pickle wrote:- Put all you functions together at the bottom or top of the file
- Put all your require() statements at the top.
- Don't jump in & out of PHP - use heredoc's if you've got big chunks of text
- Put all your initialization statements & logic together.
About the heredoc - prefer to use it liberally these days. Jumping in and out of PHP is more templte-like IMO since PHP is just doing the logic and nothing else. If you're using PHP as a template language itself it seems cleaner to me to do things like:

Code: Select all

<table>

<?php

foreach ($view->getUsers() as $user)
{

    ?>

    <tr>
        <td> <?= $user ?> </td>
    </tr>

    <?php

}

?>

</table>
Like arborint says, I'd just go over the code, trying to find areas that perform a specific task - however small - and then at least contemplate whether or not it's worth separating it into either a function/method or a new file. It's way too easy to go nuts on refactoring though and (to state the obvious) this just adds unneccesary complexity. All in all what you have at the moment looks pretty good to me ;)

Posted: Mon May 15, 2006 2:30 pm
by Christopher
It is really interesting to me to see this kind of script, because it has all the parts of any application. The upside of a script like this is that it is very efficient, very obvious, and very flexible. The downside is that the dependencies are not enforced with interfaces, but are enforced by programmer discipline. I think most PHP programmers can stay discipined at this scale.

It is pretty clear that you would add modularity to enforce the dependencies -- reducing the possiblity for hacks and cruft. And that a controller architecture would all this code to scale. You obviously wouldn't want to keep adding if statements for tens/hundreds of actions.

Ambush Commander? Are you interested in refactoring this script step by step to add modularity and then make it controller based to show the process?

Posted: Mon May 15, 2006 3:01 pm
by Ambush Commander
Is it hard to maintain? It looks fairly clean to me.
Well, I'm afraid that some time in the future it will be more difficult to maintain. Trying to head off the catastrophe, so-to-speak.
The top is config stuff, but there is also config stuff further on. That could be consolidated and simplified because it seems to be spread out. The local setting could probably be handled with the normal config stuff.
Sounds good. Perhaps the configuration should be in a different file?
You have a pre-filter that recompiles. You could move that to an include so you only parse the code when needed.
Hmm... would it really make that much of an execution difference? I'd probably move it to another file by virtue of moving out some really "out there" functionality out of the main body.
Pretty standard front controller code already, but you always do routing logic checks rather than moving the page specific stuff into the individual page classes
I'm not sure what you mean by this: create classe s the represent different pages that act as further controllers?
Finally you have the response stuff. Probably hierarchical templates would clean this up. I like a Response class because it can have standard layout defaults that most pages use, but they can be overridden. And because that stuff is not loaded until the end there is little cost for the flexiblity.
So each logical "block" of the page gets its own HTML code, and then you assemble it together. This also would allow the skin recompilation to happen without being coupled to the creation of a response. What would a response class look like?
Put all you functions together at the bottom or top of the file

Put all your require() statements at the top.

Put all your initialization statements & logic together.
Good ideas.
Don't jump in & out of PHP - use heredoc's if you've got big chunks of text
Now, that's an interesting piece of advice, and I'm glad d11wtq backed it up, because otherwise I would have been somewhat skeptical. This is what I gather: If I clearly demarcate the end of the PHP processing and the beginning of the template, jumping out of PHP is okay. Otherwise, use heredoc?
It is really interesting to me to see this kind of script, because it has all the parts of any application. The upside of a script like this is that it is very efficient, very obvious, and very flexible. The downside is that the dependencies are not enforced with interfaces, but are enforced by programmer discipline. I think most PHP programmers can stay discipined at this scale.

It is pretty clear that you would add modularity to enforce the dependencies -- reducing the possiblity for hacks and cruft. And that a controller architecture would all this code to scale. You obviously wouldn't want to keep adding if statements for tens/hundreds of actions.
Thanks for the compliment. I agree that the specific page controllers for each different action are probably the most flagrant problems on the page. It occurs to me that each of these could be represented as a "Page" class, which would determine whether or not it was dead end, or that further controller work is needed.
Are you interested in refactoring this script step by step to add modularity and then make it controller based to show the process?
Yeah, that's probably the direction I'm going in. The idea is to make things as easy to add as possible, if that needs good architecture, then good architecture it is. The whole reason why I'm using a home made CMS is because everything else out there is, to say the least, overkill.

Response has been overwhelmingly helpful. I'll be sure to do this again. :D

Posted: Mon May 15, 2006 3:06 pm
by pickle
Ambush Commander wrote:This is what I gather: If I clearly demarcate the end of the PHP processing and the beginning of the template, jumping out of PHP is okay. Otherwise, use heredoc?
There were 2 reasons for that suggestion. 1: I personally find it difficult to read code that jumps in & out of PHP. There's really no need to do that. 2: I think there is a bit of overhead when transiting between text & PHP. I'm not sure how significant it is (negligable for that script), but it's there nonetheless.

Posted: Mon May 15, 2006 3:14 pm
by Ambush Commander
I think there is a bit of overhead when transiting between text & PHP.
I thought there was a bit of overhead is using heredoc: I make it a point not to use heredoc (partially because it disrupts indenting):

Code: Select all

$registration->email->sendMessage(
              'Taiji registration #' . $registration->id,
              'You have been successfully registered to ' . $meeting->name . '. '.
              'Please send in your payment of $25 dollars to:' . "\r\n\r\n" .
              "ADDRESS\r\n\r\n".
              'Make checks payable to NAME. Make sure you include your '.
              'full name. Your registration ID is ' . $registration->id . ', '.
              'please include with your payment.'."\r\n\r\n" . 'On-site '.
              'payment is $30 and a bounced check will incur a surcharge of '.
              '$15.');