bad to good programming !

ajoo · October 9, 2016

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>";


?>

Destramic · October 9, 2016

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 October 9, 2016 by Destramic

ajoo · October 9, 2016

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 !

ginerjm · October 9, 2016

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 October 9, 2016 by ginerjm

kicken · October 10, 2016

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.

ajoo · October 10, 2016

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.

kicken · October 10, 2016

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>

ajoo · October 10, 2016

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?

Destramic · October 10, 2016

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 October 10, 2016 by Destramic

kicken · October 10, 2016

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 = '/';

Sign In

bad to good programming !

Recommended Posts

ajoo

Link to comment

Share on other sites

Destramic

Link to comment

Share on other sites

ajoo

Link to comment

Share on other sites

ginerjm

Link to comment

Share on other sites

kicken

Link to comment

Share on other sites

ajoo

Link to comment

Share on other sites

kicken

Link to comment

Share on other sites

ajoo

Link to comment

Share on other sites

Destramic

Link to comment

Share on other sites

kicken

Link to comment

Share on other sites

Join the conversation

Browse

Activity

Important Information