PHP Developers Network

A community of PHP developers offering assistance, advice, discussion, and friendship.
 
Loading
It is currently Mon Oct 22, 2018 8:27 am

All times are UTC - 5 hours




Post new topic Reply to topic  [ 6 posts ] 
Author Message
 Post subject: Coding Critique Please.
PostPosted: Sat Oct 22, 2011 4:36 pm 
Offline
Forum Contributor

Joined: Sat Oct 22, 2011 10:50 am
Posts: 159
Syntax: [ Download ] [ Hide ]
<html>
<head>
<meta content="yes" name="apple-mobile-web-app-capable" />
<meta content="text/html; charset=iso-8859-1" http-equiv="Content-Type" />
<meta content="minimum-scale=1.0, width=device-width, maximum-scale=0.6667, user-scalable=no" name="viewport" />
<link href="css/style.css" type="text/css" rel="stylesheet" />
<script src="javascript/functions.js" type="text/javascript"></script>
<link rel="apple-touch-icon" href="homescreen.png"/>
<link href="startup.png" rel="apple-touch-startup-image" />
</head>
<div id="topbar">
  <div id="title">Duty <?php echo $_REQUEST[duty_number]?></div>
</div>
<?php
//Test if it is a shared client
if (!empty($_SERVER['HTTP_CLIENT_IP'])){
  $ip=$_SERVER['HTTP_CLIENT_IP'];
//Is it a proxy address
}elseif (!empty($_SERVER['HTTP_X_FORWARDED_FOR'])){
  $ip=$_SERVER['HTTP_X_FORWARDED_FOR'];
}else{
  $ip=$_SERVER['REMOTE_ADDR'];
}
$con = mysql_connect("","","");
mysql_select_db("ARRIVA_DUTIES", $con);
$sql="INSERT INTO tbl_requests (request_ip, request_type, request_value, request_referer)
VALUES
('$ip', 'Duty', '$_REQUEST[duty_number]', '$_SERVER[HTTP_REFERER]')"
;

if (!mysql_query($sql,$con))
  {
  die('Error: ' . mysql_error());
  }
mysql_close($con)
?>
<?php
$con = mysql_connect("","","");
if (!$con)
  {
  die('Could not connect: ' . mysql_error());
  }
mysql_select_db("ARRIVA_DUTIES", $con);
$result = mysql_query("SELECT duty_type, duty_start, duty_finish, duty_paytime FROM tbl_duties WHERE duty_number = '$_REQUEST[duty_number]'");
while($row = mysql_fetch_array($result))
  {
echo "<div align='center'><table border='0' width='90%' cellpadding='1' cellspacing='2'>";
echo "<tr>
                <td>&nbsp;&nbsp;<b>Duty Type</b></td>"
;
echo    "<td>";
echo    $row[0];
echo    "</td>";
echo    "</tr>";
echo "<tr>
                <td>&nbsp;&nbsp;<b>Duty Start Time</b></td>"
;
echo    "<td>";
echo    $row[1];
echo    "</td>";
echo    "</tr>";
echo "<tr>
                <td>&nbsp;&nbsp;<b>Duty Finish Time</b></td>"
;
echo    "<td>";
echo    $row[2];
echo    "</td>";
echo    "</tr>";
echo "<tr>
                <td>&nbsp;&nbsp;<b>Duty Pay Time</b></td>"
;
echo    "<td>";
echo    $row[3];
echo    "</td>";
echo    "</tr>";
echo "</table><br>";
  }
echo "<p align='center'>If you see no results, you have entered an invalid value.<br>If you know the information here is incorrect, please report the problem using the link at the bottom of this page.</p>";
echo "</div>";
mysql_close($con);
?>
<ul class="pageitem">
  <li class="button">
    <input type="submit" value="Back" onclick='javascript&#058;history.go(-1)'"/>
  </li>
</ul>
<br>
<div id='footer'>
Developed by Mike Ashfield<br>
<a href='/problem.php'>Report a problem</a>
</div>
 


New to PHP so go easy on the techie talk. :wink:


Top
 Profile  
 
PostPosted: Sat Oct 22, 2011 5:28 pm 
Offline
Forum Regular
User avatar

Joined: Tue Sep 28, 2010 11:41 am
Posts: 984
Location: Columbus, Ohio
Here is how I would have done it (well just going off of what you gave)

Note, from a programming standpoint, I prefer to specify either $_POST or $_GET, not $_REQUEST, so that as a programmer you know what it was expecting

I didn't include it since I didn't know which you were trying for, but would have included a doctype

I try to keep as much processing before you even start HTML, helps keep the logic separate from the output (and will help you better get used to if you ever switch to full MVC). As you can see in my example, it is much easier to not only see, but change the output later.

Syntax: [ Download ] [ Hide ]
<?php

        $con = mysql_connect("","","")
                or die ('ERR: Cannot connect to Database Server');
        mysql_select_db("ARRIVA_DUTIES", $con)
                or die ('ERR: Cannot use the specified database');

        // Would use only the one needed
        $strDutyNumber = (isset($_POST['duty_number'])) ? $_POST['duty_number'] : '';
        $strDutyNumber = (isset($_GET['duty_number'])) ? $_GET['duty_number'] : '';

        if (isset($_SERVER['HTTP_CLIENT_IP']) && $_SERVER['HTTP_CLIENT_IP']!='') {
                $strIP = $_SERVER['HTTP_CLIENT_IP'];
        }
        elseif (isset($_SERVER['HTTP_X_FORWARDED_FOR']) && $_SERVER['HTTP_X_FORWARDED_FOR']!='') {
                $strIP = $_SERVER['HTTP_X_FORWARDED_FOR'];
        }
        else {
                $strIP = $_SERVER['REMOTE_ADDER'];
        }

        $SQL  = 'INSERT INTO `tbl_requests` SET ';
        $SQL .= '`request_ip`="'.$strIP.'",';
        $SQL .= '`request_type`="Duty",';
        $SQL .= '`request_value`="'.mysql_real_escape_string($strDutyNumber).'",';
        $SQL .= '`request_referer`="'.mysql_real_escape_string($_SERVER[HTTP_REFERER]).'"';

        mysql_query($SQL)
                or die('Error: ' . mysql_error());

        $SQL  = 'SELECT `duty_type`,`duty_start`,`duty_finish`,`duty_paytime` ';
        $SQL .= 'FROM `tbl_duties` WHERE duty_number = "'.mysql_real_escape_string($strDutyNumber).'"';
       
        $rsData = mysql_query($SQL);
        // NOTE: Data retrieved via HTML section
       
        function echoHSC($strText) { echo htmlspecialchars($strText); }

?><html>
<head>
        <meta name="apple-mobile-web-app-capable" content="yes" />
        <meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1" />
        <meta name="viewport" content="minimum-scale=1.0, width=device-width, maximum-scale=0.6667, user-scalable=no" />
        <link type="text/css" rel="stylesheet" href="css/style.css" />
        <script type="text/javascript" src="javascript/functions.js"></script>
        <link rel="apple-touch-icon" href="homescreen.png" />
        <link rel="apple-touch-startup-image" href="startup.png" />
</head>
<body>
        <div id="topbar">
                <div id="title">Duty <?php echoHSC($strDutyNumber); ?></div>
        </div>

        <div align="center">
                <?php if ($rsData && mysql_num_rows($rsData)>0): ?>
                        <?php while ($aryRow = mysql_fetch_assoc($rsData)): ?>
                                <table border="0" width="90%" cellpadding="1" cellspacing="2">
                                        <tr>
                                                <th>&nbsp;&nbsp;Duty Type</th>
                                                <td><?php echoHSC($aryRow['duty_type']); ?></td>
                                        </tr>
                                        <tr>
                                                <th>&nbsp;&nbsp;Duty Start Time</th>
                                                <td><?php echoHSC($aryRow['duty_start']); ?></td>
                                        </tr>
                                        <tr>
                                                <th>&nbsp;&nbsp;Duty Finish Time</th>
                                                <td><?php echoHSC($aryRow['duty_finish']); ?></td>
                                        </tr>
                                        <tr>
                                                <th>&nbsp;&nbsp;Duty Pay Time</th>
                                                <td><?php echoHSC($aryRow['duty_paytime']); ?></td>
                                        </tr>
                                </table><br>
                        <?php endwhile; // END: Loop though database results ?>
                <?php else: ?>
                        <p>There were no records found.</p>
                <?php endif; // END: Was there rows returned ?>

                <p align="center">
                        If you see no results, you have entered an invalid value.<br>
                        If you know the information here is incorrect, please report the problem using the link at the bottom of this page.
                </p>
        </div>
        <ul class="pageitem">
                <li class="button">
                        <input type="submit" value="Back" onclick="javascript&#058;history.go(-1)" />
                </li>
        </ul>
        <br>
        <div id="footer">
                Developed by Mike Ashfield<br>
                <a href="/problem.php">Report a problem</a>
        </div>
</body>


Top
 Profile  
 
PostPosted: Tue Oct 25, 2011 1:31 am 
Offline
DevNet Resident
User avatar

Joined: Sun Sep 03, 2006 5:19 am
Posts: 1579
Location: Sofia, Bulgaria
@twinedev: Much better indeed. You forgot to escape the IP address, and you're not right about $_REQUEST though :) Input data should also be explicitly cast to int/string and echo HCS should specify ENT_QUOTES and encoding.

@mikeashfield: If you really want to keep the IP address of the visitor, don't pick one, store them all. Otherwise I can easily spoof it.


Top
 Profile  
 
PostPosted: Tue Oct 25, 2011 8:02 am 
Offline
Forum Regular
User avatar

Joined: Tue Sep 28, 2010 11:41 am
Posts: 984
Location: Columbus, Ohio
Mordred wrote:
@twinedev: Much better indeed. You forgot to escape the IP address, and you're not right about $_REQUEST though :) Input data should also be explicitly cast to int/string and echo HCS should specify ENT_QUOTES and encoding.


Yup, forgot the fact that it could be something other than REMOTE_ADDR when i did that line. What did you mean about the $_REQUEST though?


Top
 Profile  
 
PostPosted: Tue Oct 25, 2011 8:08 am 
Offline
DevNet Resident
User avatar

Joined: Sun Sep 03, 2006 5:19 am
Posts: 1579
Location: Sofia, Bulgaria
The only bad instances of using $_REQUEST instead of forcing $_POST is in a login form (to make sure your code doesn't leave traces of the user credentials in the server access logs) and some similar security concerns. In all other cases it's a feature and a damn good one too.

What are your arguments against it? People usually whine about $_REQUEST citing HTTP semantics on GET and POST and whatnot, but these semantics have been broken a long time ago.


Top
 Profile  
 
PostPosted: Thu Oct 27, 2011 2:12 am 
Offline
Forum Regular
User avatar

Joined: Tue Sep 28, 2010 11:41 am
Posts: 984
Location: Columbus, Ohio
Like I mentioned, I prefer using POST and GET so that if I have to go into code later on (or I am looking at someone else's or they are looking at mine), the know was that bit meant to use a post data or something passed via query string. (especially if they post the form to a URL with a query string. Same reason I use $intVariable, $strVariable so when looking though a a ton of code, know just by looking what could be expected.

As I mentioned in my first post, just something I prefer especially after years at having to work on code other people have written that are not small scripts. (I was gonna be a smart*** and say that I was lazy and that POST and GET are less typing than REQUEST, but then that would blow away me saying I use $intVariable instead of simpler $iVariable ;-)

-Greg


Top
 Profile  
 
Display posts from previous:  Sort by  
Post new topic Reply to topic  [ 6 posts ] 

All times are UTC - 5 hours


Who is online

Users browsing this forum: No registered users and 1 guest


You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot post attachments in this forum

Jump to:  
Powered by phpBB® Forum Software © phpBB Group