Coding Critique Please.

Coding Critique is the place to post source code for peer review by other members of DevNetwork. Any kind of code can be posted. Code posted does not have to be limited to PHP. All members are invited to contribute constructive criticism with the goal of improving the code. Posted code should include some background information about it and what areas you specifically would like help with.

Popular code excerpts may be moved to "Code Snippets" by the moderators.

Moderator: General Moderators

Post Reply
mikeashfield
Forum Contributor
Posts: 159
Joined: Sat Oct 22, 2011 10:50 am

Coding Critique Please.

Post by mikeashfield »

Code: Select all

<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: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:
User avatar
twinedev
Forum Regular
Posts: 984
Joined: Tue Sep 28, 2010 11:41 am
Location: Columbus, Ohio

Re: Coding Critique Please.

Post by twinedev »

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.

Code: Select all

<?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:history.go(-1)" />
		</li>
	</ul>
	<br>
	<div id="footer">
		Developed by Mike Ashfield<br>
		<a href="/problem.php">Report a problem</a>
	</div>
</body>
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Re: Coding Critique Please.

Post by Mordred »

@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.
User avatar
twinedev
Forum Regular
Posts: 984
Joined: Tue Sep 28, 2010 11:41 am
Location: Columbus, Ohio

Re: Coding Critique Please.

Post by twinedev »

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?
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Re: Coding Critique Please.

Post by Mordred »

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.
User avatar
twinedev
Forum Regular
Posts: 984
Joined: Tue Sep 28, 2010 11:41 am
Location: Columbus, Ohio

Re: Coding Critique Please.

Post by twinedev »

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
Post Reply