Sloppy code?

3raser · March 24, 2011

Hello, recently I've been worrying about the structure and organization of my code. And I though, if I were ever to release my scripts or help out on other PHP related projects, I'd need to make sure my code is understandable, organized, and not creating extra useless processes. So here I am, asking for ALL feedback on my code, and what I should & shouldn't do to improve it:

<?php 
session_start();
include_once("includes/header.php");
include_once("includes/db_connect.php"); 
?>

<div id="main">

<div id="portal_head"></div> 
		<div id="portal"> 
		<div id="portal_main">
		<div id="portal_l">
		<h2>Trackin </h2>
		<div class="portal_box"></div>
		Here you can see the replies of support staff, and the current
		status of your ticket!
		</div>
		<div id="portal_news">	


		<div class="portal_box"> 
		<h2>Tracking</h2> 
		<div class="portal_content">
		<?
		if(!$_GET['id'] && $_POST['id'])
		{
		$id = $_POST['id'];
		}
		elseif($_GET['id'] && !$_POST['id'])
		{
		$id = $_GET['id'];
		}
		else
		{
		//nothing
		}


			if(!$id)
			{
				echo "Hold up there! Seems you haven't submitted a tracking ID!";
			}
			else
			{
				$does_exist = mysql_query("SELECT title,question,status FROM tickets WHERE tracking = '$id'");

				if(mysql_num_rows($does_exist) == 0)
				{
					echo "WOOPS! Seems no ticket exists with that tracking ID!";
				}
				else
				{
				if(!$_POST['message'])
				{


					$replies = mysql_query("SELECT message,`by` FROM replies WHERE replyto = '$id' ORDER BY id DESC") or die(mysql_error());
					$extract = mysql_fetch_assoc($does_exist);

					switch ($extract['status'])
					{
					case 0:
					$status = "Unresolved.";
					break;

					case 1:
					$status = "Locked.";
					break;

					case 2: 
					$status = "Solved.";
					break;
					}

					echo "<h2>". $extract['title'] ."</h2><table><tr><td>Status</td><td>". $status ."</td></tr></table>";


					while($e_rep = mysql_fetch_assoc($replies))
					{
						echo "<hr>". $e_rep['by'] ." says: ". $e_rep['message'] ."<hr>";
					}
					}
					else
					{
						$message = mysql_real_escape_string($_POST['message']);

						if(!$_SESSION['admin'])
						{
							mysql_query("INSERT INTO replies VALUES (null, '$message', 'Guest', '$id')");
						}
						else
						{
							mysql_query("INSERT INTO replies VALUES (null, '$message', 'Support Team Member', '$id')");
						}
					}
				}
			}

		?>
		</div> 
		</div>	



		</div>
		<div id="portal_clear"> </div>			
		</div>
		</div>
		</div>
</body></html>

joel24 · March 24, 2011

I suggest you separate the php from the html, execute the php code at the top of the page and then assign any html it produces to a variable and call it from the html... to keep things clean and easy to follow.

And another thing, when you post here use [ php ] tags rather than [ code ]

**edit** my bad, you did use [ php ], just you used the shorthand <? opener, you should use the full <?php

i.e.

<?php
//php code
$myHTML = "this is the html";
?>
<html>
<body>
<div>
<?php echo $myHTML; ?>
</div>
</body>
</html>

QuickOldCar · March 24, 2011

It's not bad to break in and out of php/html. Sometimes it's just easier.

Many big cms do it that way.

I personally like to keep everything related in one clump of the code, that would be any functions,rules and rendering output. Just in case I want to completely remove it one day, I hate to search for stuff.

I feel is more of a preference really.

And i also agree not to use the short tags <?, use <?php

3raser · March 25, 2011

Thanks for the replies. I will no longer use <? ?>, thanks for the tip!

But using PHP within HTML seems like it would run faster if I'm not mistaken, correct? Also wouldn't need to set all these extra variables just to output text, etc.

btherl · March 25, 2011

While mixing php and html is not always bad, I believe it's important to seperate decision making code from the display code. For example, your php code at the top should decide which page display and what data needs to go on it. Then in a seperate section below, the HTML is generated. No decisions should be made in the HTML section - it should rely entirely on decisions made by the code above.

Exceptions to this rule would be simple decisions solely related to display, such as "If this variable is more than 30 characters long, shorten it and add a mouseover". This is purely display related, and importantly it can never fail. Errors should all have been handled by the time we get to the HTML section.

Data fetching and form processing code should also be entirely in the PHP section, and there are no exceptions here.

The advantage of all this is that your code will be easier to maintain. Even major changes to both the decision and the display code will be simpler, because each can be modified independently of the other.

Psycho · March 25, 2011

Here are my suggestions:

1. Never create your queries directly in the mysql_query() function. Create them as a variable and use the variable in the mysql_query() function. If you ever have a problem you will be able to quickly debug it be echoing out the query. Or even better you can log the error and the query. That way if a situation ever occurs in production that was accounted for in testing you would be able to trace teh problem

2. Be consistent with your indentations. You do a fairly good job, but there inconsistencies

3. Comment, comment, comment. There's nothing worse than spending a couple hours to come up with some really elegant code to handle a complex scenario. Then coming back to that code weeks/months later to do a bug fix or enhancement and spending another two hours to understand the code you wrote.

4. This code

switch ($extract['status'])
{
case 0:
$status = "Unresolved.";
break;

case 1:
$status = "Locked.";
break;
                  
case 2: 
$status = "Solved.";
break;
}

Could have been simplified to this:

$statuses = array("Unresolved.", "Locked.", "Solved.");
$status = $statuses[$extract['status']];

5. Error handling. You have absolutely no error handling for your queries.

6. Escaping/cleansing user input!!! You are using data sent by the user directly in your database queries. That is a HUGE security risk.

7. This

if(!$_SESSION['admin'])
{
    mysql_query("INSERT INTO replies VALUES (null, '$message', 'Guest', '$id')");
}
else
{
    mysql_query("INSERT INTO replies VALUES (null, '$message', 'Support Team Member', '$id')");
}

Could have been simplified to this

$user = (!$_SESSION['admin']) ? 'Guest' : 'Support Team Member';
mysql_query("INSERT INTO replies VALUES (null, '$message', '$user', '$id')");

3raser · March 26, 2011

Here are my suggestions:

1. Never create your queries directly in the mysql_query() function. Create them as a variable and use the variable in the mysql_query() function. If you ever have a problem you will be able to quickly debug it be echoing out the query. Or even better you can log the error and the query. That way if a situation ever occurs in production that was accounted for in testing you would be able to trace teh problem

2. Be consistent with your indentations. You do a fairly good job, but there inconsistencies

3. Comment, comment, comment. There's nothing worse than spending a couple hours to come up with some really elegant code to handle a complex scenario. Then coming back to that code weeks/months later to do a bug fix or enhancement and spending another two hours to understand the code you wrote.

4. This code
switch ($extract['status'])
{
case 0:
$status = "Unresolved.";
break;

case 1:
$status = "Locked.";
break;
                  
case 2: 
$status = "Solved.";
break;
}
Could have been simplified to this:
$statuses = array("Unresolved.", "Locked.", "Solved.");
$status = $statuses[$extract['status']];
5. Error handling. You have absolutely no error handling for your queries.

6. Escaping/cleansing user input!!! You are using data sent by the user directly in your database queries. That is a HUGE security risk.

7. This
if(!$_SESSION['admin'])
{
    mysql_query("INSERT INTO replies VALUES (null, '$message', 'Guest', '$id')");
}
else
{
    mysql_query("INSERT INTO replies VALUES (null, '$message', 'Support Team Member', '$id')");
}
Could have been simplified to this
$user = (!$_SESSION['admin']) ? 'Guest' : 'Support Team Member';
mysql_query("INSERT INTO replies VALUES (null, '$message', '$user', '$id')");

Wow, this feedback is greatly appreciated.

But first, to the poster above

====================

I might start using a variable to echo the content so the PHP isn't mixed with the HTML. But does this slow down the code any? But I know it also gives benefits for no header errors....

====================

Now to mjdamato:

1) Well the keyword here is testing, correct? I always run tests before I make the content go live. So I know for a fact the queries work, thus the reason why you see no error catching for those queries, because they've already been tested and given a green stamp.

2) Thanks, will keep in mind.

3) Thanks, I will start doing this! Makes sense.

4) Thanks, I've never really looked into Arrays, even though I really should. :/ - I will start using that format from now on.

5) Same answer for #1.

6) What do you mean by this? I do use mysql_real_escape_string to add protection to the input from the users.

7) Same answer for #4.

Other than that, I really appreciate the feedback. And I hope you can reply to my open-ended questions, such as: #1, #5, #6

3raser · March 26, 2011

Bump!

3raser · March 27, 2011

Can anyone answer those questions? Thanks!

nethnet · March 27, 2011

The particular chunk of code you posted has a lot of if/else statements, many of which consist of only one line. While this is a matter of preference, it can be done to shorten your code and remove some of those rogue lines with nothing more than a '{':

if ($something == 1)
     echo "It's 1!";
else if ($something == 2)
     echo "It's 2 this time";
else {
     echo "Nothing!";
     $string = "we failed";
}

Braces are only necessary around chunks of code more than 1 line. As I said, this is a matter of preference, but I personally think code looks much cleaner when braces are only used when absolutely necessary (maybe because I'm a Ruby programmer?)

Also, mjdamato pointed out the ternary operator to make some of your if/else statements much more concise and presentable. I would recommend looking into that more if you don't know exactly how it is used.

chris.smith · March 27, 2011

Well for a start you could use a PHP Beautifier

Check this out.

Cheers.

btherl · March 27, 2011

I might start using a variable to echo the content so the PHP isn't mixed with the HTML. But does this slow down the code any? But I know it also gives benefits for no header errors....

Any slow-down due to echoing a variable instead of having code in the HTML itself is negligible. Most of the time will be spent in fetching database data, processing the data and parsing the script. Memory usage due to having to store all data until HTML is displayed is usually not an issue as HTML pages tend to be small, and use paging for large data sets.

To speed up fetching of database data, the first step is to optimize your queries, including adding (and removing) indexes. If that fails, you can use a caching layer like memcache (Facebook uses this). Processing of data can be sped up by choosing better algorithms and data structures, if some are available. Parsing time can be eliminated by using a cache such as Xcache. All of these don't really matter unless you are experiencing slow response time though. Optimizing before there is a problem often causes more trouble than it fixes, because you don't know where the optimizing is needed when there's no problems.

In answer to your error checking question - no, you still need error checking. Queries can fail due to the database crashing, or someone altering the database, or the database getting corrupted, or someone tripping over a network cable, or a million other reasons beyond your control. Your script's job is to take appropriate action, which is usually to send an email to you reporting the error message, and to tell the user that something went wrong and the administrator has been notified. If you want you can make a "my_mysql_query()" wrapper which does all this checking for you. Then you just replace every mysql_query() with your wrapper, and save yourself some time.

Regarding santizing of user input destined for mysql queries, your first db query does not escape $id. If you know it should be a number, then verify that first - check for any non-numeric characters and give an error if you find one. Then you can use the sanitized number directly in your query. For strings it is the same, but you have a choice - either restrict the character set so that no dangerous character can be present, or use mysql_real_escape_string(). In both case, it's better practice to validate the character set and give an error first instead of passing the invalid data through to the mysql query and getting the error there. BTW this ties in with your already-tested mysql queries - bad user provided data might make these fail, or even worse, do something else. Eg if I give your script "id=';DROP TABLE users;--" then you query will be this:

SELECT title,question,status FROM tickets WHERE tracking = '';DROP TABLE users;--'

3raser · March 27, 2011

All of the replies above are very helpful, and I'll look more into the if & else statement suggestions. But not using brackets, can't they make you lose track? And I have yet to see PHP scripts that use a system like such. But if I could get more opinions on it, I'd be happy to give it a shot.

======

@btherl

Thanks for all of that! I will now start officially putting my PHP code at the top of the page, and just use variables to show the data. Also, how your MySQL injection statement, the dropping, only $id is vulnerable, correct? Because if something doesn't go through error checking, say die(mysql_error()), how can my database still risk MySQL injection if it's gone through mysql_real_escape_string? I thought the error checking process wasn't for security, but more of notifications and reporting faulty databases/problems.

btherl · March 28, 2011

I don't recommend leaving out brackets. I very rarely do it myself, even if it's just a single line. It's more a matter of taste. I often leave the brackets out if I put debugging code on a single line (and I often indent debugging code to the left of real code, so I know it's to be ignored).

Back to error checking - yes you are right. That's what I was trying to say, though I might not have been clear As long as you use mysql_real_escape_string() and put the result in quotes in the query, you are safe from injection, even if you don't check for errors from mysql_query(). But if you do check for errors from mysql_query() and have it email you both the original query and mysql_error(), then you will be able to identify and fix the problem quickly.

Psycho · March 28, 2011

Now to mjdamato:

1) Well the keyword here is testing, correct? I always run tests before I make the content go live. So I know for a fact the queries work, thus the reason why you see no error catching for those queries, because they've already been tested and given a green stamp.

5) Same answer for #1.

To think that you you tested your queries for every eventuality is ridiculous. I have worked for multiple software companies and I have seen multiple instances where a query passed all initial tests and then some time int he future it fails because of a particular set of circumstances. Or, there is the situation where someone modifies some other code or the database such that previously existing queries fail. It doesn't take a great deal of effort to create a bit of error handling code to make your application a solid product. Typically I create a custom function for running queries to use in place of mysql_query(). That way I can specify different error handling based upon whether the application is in debug mode or production mode. Then you don't have to write error handling for each and every query.

6) What do you mean by this? I do use mysql_real_escape_string to add protection to the input from the users.

I don't see where you used mysql_real_escape_string() on the user input (i.e. "id"). I only see you using it on the message value which you obtained from a previous db query. That is a good idea since it may contain characters that would foul the query, but if "message" contained malicious code it would have been run when the record was inserted. Although I didn't specify this before, you are also not escaping the code you print to the page.

echo "<hr>". $e_rep['by'] ." says: ". $e_rep['message'] ."<hr>";

That could lead to a user unintentionally breaking your page if they inserted code that could be interpreted as HTML. Or worse, it opens you up to XSS attacks.

Sign In

Sloppy code?

Recommended Posts

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Join the conversation

Important Information