Jump to content

bad to good programming !


ajoo

Recommended Posts

Hi all, 

 

Below is a small example snippet of an enmeshed php, html, css & js program which I think would be an example of bad programming.

Using this simple example below can someone please demonstrate how it may be used to separate the code and convert the html into a template. It would be a nice example to learn about templating I think. Thank all.  

 



<?php

print_r($_POST);
$rooms = array(1,2,3,4);
$displayed = 2;
?>
<script type="text/javascript" src="https://ajax.googleapis.com/ajax/libs/jquery/1.3.2/jquery.js"></script>

<script type="text/javascript">
$(document).ready(function(){
$('#room').change(function(){
$(this).parent('form').submit();
});
});
</script>

<?php

echo "<div>";
echo "<form id = 'form1' action='#' method='post' > ";
echo "<select name='room' id='room'>";
if(isset($displayed)) echo "<option selected>".$displayed."</option>";
$i = 0;

while($i < count($rooms))
{
$room = $rooms[$i];
if($room === $displayed)echo "";
else echo "<option value = ".$room." > ".$room." </option>";
$i++;
}
echo "</select>";
echo "<noscript><input type='submit' value='Submit'></noscript>";
echo "</form>";
echo "</div>";

?>



 

 

Link to comment
Share on other sites

1. its not a valid html document

2. why echo html when there is no need?

<div> 
<form id = 'form1' action='#' method='post' >
<select name='room' id='room'>

<?php
if(isset($displayed)) echo "<option selected>".$displayed."</option>";
$i = 0;

while($i < count($rooms))
{ 
$room = $rooms[$i];
if($room === $displayed)echo ""; 
else echo "<option value = ".$room." > ".$room." </option>";
$i++;
}
?>

</select>
<noscript><input type='submit' value='Submit'></noscript>
</form>
</div>

3. i don't really understand you question, i can only guess your looking for something like http://twig.sensiolabs.org/

 

you could also put

$(document).ready(function(){ 
$('#room').change(function(){
$(this).parent('form').submit();
});
});

into a .js file and include as a script like your jquery

if($room === $displayed)echo "";

just seems unnecessary

 

something like this would make more sence

while($i < count($rooms))
{ 
    $room = $rooms[$i];
    
    if($room !== $displayed) echo "<option value = ".$room." > ".$room." </option>";
    $i++;
}

and try and make your code presentable?

<?php
print_r($_POST);
$rooms     = array(1, 2, 3, 4);
$displayed = 2;
?>
<script type="text/javascript" src="https://ajax.googleapis.com/ajax/libs/jquery/1.3.2/jquery.js"></script>
<script type="text/javascript">
$(document).ready(function(){ 
    $('#room').change(function(){
        $(this).parent('form').submit();
    });
});
</script>
<div>
<form id = 'form1' action='#' method='post' >
<select name='room' id='room'> 
<?php
if(isset($displayed)) echo "<option selected>".$displayed."</option>";
$i = 0;

while($i < count($rooms))
{ 
    $room = $rooms[$i];
    
    if($room !== $displayed) echo "<option value = ".$room." > ".$room." </option>";
    $i++;
}
?>
</select>
<noscript><input type='submit' value='Submit'></noscript>
</form>
</div>

Edited by Destramic
Link to comment
Share on other sites

Hi Destramic, 

 

Thanks for the response. 

 

 

 

its not a valid html document

Hmm It's a part of an HTML document which I have modified slightly. The php around this bit of code is because of the need to evaluate the value of rooms from a DB. ( not shown here) and then populate the same. It's those values that should be calculated else where ( in the pure php part of the doc ) and then sent thru here. I think that's what templating is able to accomplish.  ( That's what I could make out from some of questions involving templates on the forum)

 

The js, ofcourse would go into it's own file. That was just to put together a working example file.

if($room === $displayed)echo "";

 

 

just seems unnecessary

 

hmm again there was a session value that was to be echoed here.

 

 

 

and try and make your code presentable?

 

 Something like that I intend to do after learning a bit about templating, 

 

Thanks loads !

Link to comment
Share on other sites

The first thing that would improve it is to completely separate your processes. THINK about your html code and place that at the bottom of your script and insert php vars where you think you will have some dynamically-generated output to be displayed. THEN go back to the top of your script and start handling the task. Receive the input, validate it, use it and then execute the output of your html code. (Personally I use a function to do my entire html output that I call when I'm ready to send it.) Place your JS code either in a separate file to be included by the html portion, or place it in this separated html portion. Same for your css code. Keep the php separate! YOu may have some php/html mixed when you are building a table of output or a dropdown list but that is necessary. The idea is to keep all the static html and css and js away from the logic process.

Edited by ginerjm
Link to comment
Share on other sites

What you want to do is separate your HTML and your PHP code as much as possible. For larger applications this probably would involve keeping the HTML in a separate file and using a template engine such as TWIG to process it and render the final result. For smaller single-page scripts you can generally keep all your PHP code at the top and the HTML at the bottom, possibly using functions to create separate re-usable blocks of HTML.

 

Your specific example isn't really enough to give much advice. One alternative way to write it is this:

<?php
// I am assuming these are something you gather from a database or somewhere
$rooms = [1,2,3,4];
$displayed = 2; 
//--------

?>
<script type="text/javascript" src="https://ajax.googleapis.com/ajax/libs/jquery/1.3.2/jquery.js"></script>
<script type="text/javascript">
$(document).ready(function(){ 
    $('#room').change(function(){
        $(this).parent('form').submit();
    });
});
</script>
<div> 
<form id='form1' action='#' method='post'>
    <select name='room' id='room'>
    <?php foreach ($rooms as $room): ?>
        <option value="<?=htmlspecialchars($room)?>"
            <?=($displayed==$room)?'selected':''?>
        ><?=htmlspecialchars($room)?></option>
    <?php endforeach; ?>
    </select>
    <noscript><input type='submit' value='Submit'></noscript>
</form>
</div>
I'm assuming the $rooms and $displayed variables are coming from somewhere and not just statically set like that. When I mix HTML and PHP I prefer to use the alternative syntax for control structures as I find it much more readable. I also prefer the short syntax for echoing out variables for the same reason.

 

As a more complete example here is a script I created years ago to list the files in a directory. At one point in the past I had a directory on my server that I'd just drop lots of example/test scripts in and need it to be browsable so I setup the following as my DirectoryIndex file for that folder.

 

<?php

$showHiddenFiles=false;


$ICON_MAP = array(
    'folder' => '/icons/folder.png',
    'file' => '/icons/generic.png'
);

if (isset($_GET['viewsource'])){
    header('Content-type: text/html');
    show_source(__FILE__);
    exit;
}
else {
    chdir($_SERVER['DOCUMENT_ROOT']);

    $path = $_SERVER['REQUEST_URI'];
    $query = $_SERVER['QUERY_STRING'];
    $path = str_replace('?'.$query, '', $path);




    $realPath = rtrim($_SERVER['DOCUMENT_ROOT'], DIRECTORY_SEPARATOR);
    $pathParts = explode('/', trim($path, '/'));
    foreach ($pathParts as $part){
        $realPath .= DIRECTORY_SEPARATOR.$part;
    }

    $path=$realPath;
    if (!$path || strncmp($path, $_SERVER['DOCUMENT_ROOT'], strlen($_SERVER['DOCUMENT_ROOT']))){
        die('Invalid path');
    }

    $reqPath = substr($path, strlen($_SERVER['DOCUMENT_ROOT']));
    if ($reqPath[0] != '/'){
        $reqPath = '/'.$reqPath;
    }

    if (!is_dir($path) || !is_readable($path)){
        die('Cannot access '.$path);
    }

    $folders = $files = array();
    $dh = opendir($path);
    while (($entry=readdir($dh)) !== false){
        $fpEntry = $path.DIRECTORY_SEPARATOR.$entry;
        if ($entry != '.' && $entry != '..'){
            if (is_dir($fpEntry) && ($showHiddenFiles || $entry[0] != '.')){
                $folders[] = array(
                    'name' => $entry,
                    'full-path' => $fpEntry,
                    'last-modified' => filemtime($fpEntry),
                    'last-access' => fileatime($fpEntry),
                    'created-on' => filectime($fpEntry),
                    'permissions' => fileperms($fpEntry),
                    'size' => filesize($fpEntry),
                    'extension' => '',
                    'icon' => $ICON_MAP['folder']
                );
            }
            else if (is_file($fpEntry)){
                if ($showHiddenFiles || $entry[0] != '.'){
                    $files[] = array(
                        'name' => $entry,
                        'full-path' => $fpEntry,
                        'last-modified' => filemtime($fpEntry),
                        'last-access' => fileatime($fpEntry),
                        'created-on' => filectime($fpEntry),
                        'permissions' => fileperms($fpEntry),
                        'size' => filesize($fpEntry),
                        'extension' => ($ext=strtolower(strrchr($entry, '.'))),
                        'icon' => (isset($ICON_MAP[$ext]))?$ICON_MAP[$ext]:$ICON_MAP['file']
                    );
                }
            }
        }
    }
}


if (!isset($_GET['S'])){
    $S = 'name';
}
else {
    $S = $_GET['S'];
}

$O = (isset($_GET['O']) && in_array($_GET['O'], array('D','A')))?$_GET['O']:'A';

function SortFiles($a, $b){
    global $S, $O;

    $ret = 0;
    switch ($S){
        case 'size':
            $ret=$a['size'] - $b['size'];
            break;
        case 'modified':
            $ret=$a['last-modified'] - $b['last-modified'];
            break;
        default:
            $ret=strcmp(strtolower($a['name']), strtolower($b['name']));
    }

    return $O=='A'?$ret:($ret*-1);
}

usort($files, 'SortFiles');
usort($folders, 'SortFiles');

header('Content-type: application/xml');
?>
<?xml version="1.0" encoding="us-ascii"?>
<?xml-stylesheet type="text/xsl" href="/extras/.dirindex.xsl"?>
<page title="Index of <?=htmlspecialchars($reqPath);?>">
    <header>
        <!--<script href="/extras/.dirindex.js" />-->
        <stylesheet href="/extras/.dirindex.css" />
    </header>
    <content>
        <directoryIndex directory="<?=htmlspecialchars($reqPath);?>" sortColumn="<?=htmlspecialchars($S);?>" sortDir="<?=htmlspecialchars($O);?>">
                <breadcrumbs>
                    <?php $prefix='/'; foreach ($pathParts as $part): ?>
                    <crumb path="<?=htmlspecialchars($prefix.$part);?>" label="<?=htmlspecialchars($part);?>" />
                    <?php $prefix .= $part.'/'; endforeach;?>
                </breadcrumbs>
                <contents>
                    <?php foreach ($folders as $folder): ?>
                        <entry type="directory"
                            name="<?=htmlspecialchars($folder['name']);?>"
                            last-modified="<?=htmlspecialchars(date('r', $folder['last-modified']));?>"
                            last-access="<?=htmlspecialchars(date('r', $folder['last-access']));?>"
                            created-on="<?=htmlspecialchars(date('r', $folder['created-on']));?>"
                            permissions="<?=htmlspecialchars($folder['permissions']);?>"
                            size="<?=htmlspecialchars($folder['size']);?>"
                            icon="<?=htmlspecialchars($folder['icon']);?>"
                            id="<?=htmlspecialchars(sha1($folder['full-path']));?>"
                            />
                    <?php endforeach;?>

                    <?php foreach ($files as $file): ?>
                        <entry type="file"
                            name="<?=htmlspecialchars($file['name']);?>"
                            last-modified="<?=htmlspecialchars(date('r', $file['last-modified']));?>"
                            last-access="<?=htmlspecialchars(date('r', $file['last-access']));?>"
                            created-on="<?=htmlspecialchars(date('r', $file['created-on']));?>"
                            permissions="<?=htmlspecialchars($file['permissions']);?>"
                            size="<?=htmlspecialchars($file['size']);?>"
                            extension="<?=htmlspecialchars($file['extension']);?>"
                            icon="<?=htmlspecialchars($file['icon']);?>"
                            id="<?=htmlspecialchars(sha1($folder['full-path']));?>"
                            />
                    <?php endforeach;?>
                </contents>
        </directoryIndex>
    </content>
</page>
As you can see all the work of finding the files and sorting them is done at the top. The bottom is the generated XML with just a few loops and echos, nothing complicated.
Link to comment
Share on other sites

Hi ginerjm & kicken,

 

Thank you both for the inputs to this. 

@ginerjm : I generally do keep my php code at the top and separated from the HTML. The js goes into it's own files.

 

@kicken : I'll go through the code example and revert in sometime. You are correct in all your assumptions. The $rooms values do come from a DB. I just created those variables to simplify the code.

 

What i was looking for was a way of working my example with a templating engine like Twig as you mentioned. I believe, and I may be wrong, that the php values (like the values in the array $rooms) are somehow, through the templating engine mechanism, passed to the HTML and rendered there. I was actually looking for an example of that sort.

 

Still I am very grateful for all your inputs. I'll revert after playing around with the code and hopefully learning something nice ! Thanks.

Link to comment
Share on other sites

To use something like TWIG you need to do two things. First you need to learn how to integrate it into the PHP side of the site so you can render and output the templates. Second you need to learn the syntax twig uses to create templates.

 

Twig has some guides for both if you read their documentation

 

Re-working my directory index example results in this:

 

.dirindex.php

<?php

//Composer require twig/twig
require 'vendor/autoload.php';

$showHiddenFiles=false;


$ICON_MAP = array(
    'folder' => '/icons/folder.png',
    'file' => '/icons/generic.png'
);

if (isset($_GET['viewsource'])){
    header('Content-type: text/html');
    show_source(__FILE__);
    exit;
}
else {
    chdir($_SERVER['DOCUMENT_ROOT']);

    $path = $_SERVER['REQUEST_URI'];
    $query = $_SERVER['QUERY_STRING'];
    $path = str_replace('?'.$query, '', $path);




    $realPath = rtrim($_SERVER['DOCUMENT_ROOT'], DIRECTORY_SEPARATOR);
    $pathParts = explode('/', trim($path, '/'));
    foreach ($pathParts as $part){
        $realPath .= DIRECTORY_SEPARATOR.$part;
    }

    $path=$realPath;

    if (!$path || strncmp($path, $_SERVER['DOCUMENT_ROOT'], strlen($_SERVER['DOCUMENT_ROOT']))){
        die('Invalid path');
    }

    $reqPath = substr($path, strlen($_SERVER['DOCUMENT_ROOT']));
    if ($reqPath[0] != '/'){
        $reqPath = '/'.$reqPath;
    }

    if (!is_dir($path) || !is_readable($path)){
        die('Cannot access '.$path);
    }

    $folders = $files = array();
    $dh = opendir($path);
    while (($entry=readdir($dh)) !== false){
        $fpEntry = $path.DIRECTORY_SEPARATOR.$entry;
        if ($entry != '.' && $entry != '..'){
            if (is_dir($fpEntry) && ($showHiddenFiles || $entry[0] != '.')){
                $folders[] = array(
                    'name' => $entry,
                    'id' => sha1($fpEntry),
                    'lastModified' => filemtime($fpEntry),
                    'lastAccess' => fileatime($fpEntry),
                    'createdOn' => filectime($fpEntry),
                    'permissions' => fileperms($fpEntry),
                    'size' => filesize($fpEntry),
                    'extension' => '',
                    'icon' => $ICON_MAP['folder']
                );
            }
            else if (is_file($fpEntry)){
                if ($showHiddenFiles || $entry[0] != '.'){
                    $files[] = array(
                        'name' => $entry,
                        'id' => sha1($fpEntry),
                        'lastModified' => filemtime($fpEntry),
                        'lastAccess' => fileatime($fpEntry),
                        'createdOn' => filectime($fpEntry),
                        'permissions' => fileperms($fpEntry),
                        'size' => filesize($fpEntry),
                        'extension' => ($ext=strtolower(strrchr($entry, '.'))),
                        'icon' => (isset($ICON_MAP[$ext]))?$ICON_MAP[$ext]:$ICON_MAP['file']
                    );
                }
            }
        }
    }
}


if (!isset($_GET['S'])){
    $S = 'name';
}
else {
    $S = $_GET['S'];
}

$O = (isset($_GET['O']) && in_array($_GET['O'], array('D','A')))?$_GET['O']:'A';

function SortFiles($a, $b){
    global $S, $O;

    $ret = 0;
    switch ($S){
        case 'size':
            $ret=$a['size'] - $b['size'];
            break;
        case 'modified':
            $ret=$a['last-modified'] - $b['last-modified'];
            break;
        default:
            $ret=strcmp(strtolower($a['name']), strtolower($b['name']));
    }

    return $O=='A'?$ret:($ret*-1);
}

usort($files, 'SortFiles');
usort($folders, 'SortFiles');

header('Content-type: application/xml');

//Create a loader which handles locating and loading the template contents
$twigLoader = new \Twig_Loader_Filesystem(__DIR__);

//Create an environment which handles processing the templates.  Give it
//the loader we created so it can find the templates.
$twig = new \Twig_Environment($twigLoader);

//Render the template. First parameter is the file name, second is the
//variables you want to make available within the template
echo $twig->render('.dirindex.xml.twig', [
    'reqPath' => $reqPath
    , 'S' => $S
    , 'O' => $O
    , 'pathParts' => $pathParts
    , 'folders' => $folders
    , 'files' => $files
]);
.dirindex.xml.twig

 

<?xml version="1.0" encoding="us-ascii"?>
<?xml-stylesheet type="text/xsl" href="/templating/twig/.dirindex.xsl"?>
<page title="Index of {{reqPath}}">
	<header>
		<stylesheet href="/templating/twig/.dirindex.css" />
	</header>
	<content>
		<directoryIndex directory="{{reqPath}}" sortColumn="{{S}}" sortDir="{{O}}">
				<breadcrumbs>
					{% set prefix='/' %}
					{% for part in pathParts %}
					<crumb path="{{prefix}}{{part}}" label="{{part}}" />
					{% set prefix = prefix ~ part ~ '/' %}
					{% endfor %}
				</breadcrumbs>
				<contents>
					{% for folder in folders %}
						<entry type="directory"
							name="{{folder.name}}"
							last-modified="{{folder.lastModified|date('r')}}"
							last-access="{{folder.lastAccess|date('r')}}"
							created-on="{{folder.createdOn|date('r')}}"
							permissions="{{folder.permissions}}"
							size="{{folder.size}}"
							icon="{{folder.icon}}"
							id="{{folder.id}}"
							/>
					{% endfor %}

					{% for file in files %}
						<entry type="file"
							name="{{file.name}}"
							last-modified="{{file.lastModified|date('r')}}"
							last-access="{{file.lastAccess|date('r')}}"
							created-on="{{file.createdOn|date('r')}}"
							permissions="{{file.permissions}}"
							size="{{file.size}}"
							extension="{{file.extension}}"
							icon="{{file.icon}}"
							id="{{folder.id}}"
							/>
					{% endfor %}
				</contents>
		</directoryIndex>
	</content>
</page>
Link to comment
Share on other sites

Hi Kicken, Thanks again for the reply and the Twig example. I am sure it will be of great help in helping me learn about templating. 

 

I was in the process of checking out the code you sent earlier. However it always goes and dies at the following bit of code:-

    if (!is_dir($path) || !is_readable($path)){
        die('Cannot access '.$path);

and gives the following error:- 

 

 

Cannot access D:/xampp/htdocs\xampp\kicken\.dirindex.php

 

Any suggestions?

Link to comment
Share on other sites

use forward slashes insead of backslashes

D:/xampp/htdocs/xampp/kicken/.dirindex.php

you want to check file exists and that the file is readable

    if (!file_exists($path) && !is_readable($path)){
        die('Cannot access '.$path);
}

you using is_dir() which is checking if the $path is a directory...which it isn't, so that is why you are seening an error message

Edited by Destramic
Link to comment
Share on other sites

The script is not designed to be run directly, it's designed to be run via the DirectoryIndex directive in apache. If you just want to get it working to play with then just change

	$path = $_SERVER['REQUEST_URI'];
to some static path (relative to document root) such as

	$path = '/';
Link to comment
Share on other sites

This thread is more than a year old. Please don't revive it unless you have something important to add.

Join the conversation

You can post now and register later. If you have an account, sign in now to post with your account.

Guest
Reply to this topic...

×   Pasted as rich text.   Restore formatting

  Only 75 emoji are allowed.

×   Your link has been automatically embedded.   Display as a link instead

×   Your previous content has been restored.   Clear editor

×   You cannot paste images directly. Upload or insert images from URL.

×
×
  • Create New...

Important Information

We have placed cookies on your device to help make this website better. You can adjust your cookie settings, otherwise we'll assume you're okay to continue.