Help in Code Optimizing ( Please )

[semperfi]

Hi

 

I am trying to learn to clean coding and learning to clean old coding for any vulnerabilities. Can anyone help me with this code and see what I missed. I know theres a lot can anyone help so I can someday be able to do this myself easily please. Thanks for any and all help...

 

 

<?
include("connect.php");
        include("mail.php");
include("config.php");
        
function SendCounterMail($butlerstat1, $updatestat1) {	
	$content1 = '';

	$content1 .= "<font style='font-size: 10px;font-family: Arial, Helvetica, sans-serif;color: #333333;'>".
					 "</font><br>"."<br>".
					 "<p align='center' style='font-size: 14px;font-family: Arial, Helvetica, sans-serif;font-weight:bold;'>Counter Information</p>".
					 "<br>"."<table border='0' cellpadding='3' cellspacing='0' width='100%' align='center' class='style13'>";

	if ( $butlerstat1 ) $content1 .= "<tr style='font-size: 10px;font-family: Arial, Helvetica, sans-serif;color: #333333;'>".
												"<td>Butler file is not running so it is now running by this process.</td></tr>";

	if ( $updatestat1 ) $content1 .= "<tr style='font-size: 10px;font-family: Arial, Helvetica, sans-serif;color: #333333;'>".
												"<td>Records file is not running so it is now running by this process.</td></tr>";

	$content1 .= "<tr style='font-size: 10px;font-family: Arial, Helvetica, sans-serif;color: #333333;'>".
					 "<td>This mail is testing for check file running in.</td></tr></table>";

	$subject = "Counter Information";
	$from=$adminemailadd;//= "newuser@thissite.com";
	$email	= $adminemailadd;
	//SendHTMLMail($email, $subject, $content1, $from);
}

$butlerstat = FALSE;
$updatestat = FALSE;

$ressel = mysql_query("select referral_bids from auction_pause_management where id=3");
if ( mysql_num_rows($ressel) == 0 ) {
	mysql_free_result($ressel);

	mysql_query("Insert into auction_pause_management (referral_bids) values (1)");
	$ressel = mysql_query("select referral_bids from auction_pause_management where id=3");
}

$oldvalue1 = mysql_result($ressel, 0);
mysql_free_result($ressel);

$ressel = mysql_query("select referral_bids from auction_pause_management where id=4");
if ( mysql_num_rows($ressel) == 0 ) {
	mysql_free_result($ressel);

	mysql_query("Insert into auction_pause_management (referral_bids) values (1)");
	$ressel = mysql_query("select referral_bids from auction_pause_management where id=4");
}	

$oldvalue = mysql_result($ressel, 0);
mysql_free_result($ressel);

sleep(2);

$ressel = mysql_query("select referral_bids from auction_pause_management where id=3");
$newvalue1 = mysql_result($ressel, 0);
mysql_free_result($ressel);

$ressel = mysql_query("select referral_bids from auction_pause_management where id=4");
$newvalue = mysql_result($ressel, 0);
mysql_free_result($ressel);

//if ( $oldvalue1 == $newvalue1 ) {
	$output1 = exec("php ".getcwd()."/update_butler.php >/dev/null &");
	$butlerstat = TRUE;
//}

//if ( $oldvalue == $newvalue ) {
	$output = exec("php ".getcwd()."/update_records.php >/dev/null &");
	$updatestat = TRUE;
//}
//SendCounterMail($butlerstat, $updatestat);

//mysql_close($db);
?>

[jim_keller]

A couple of immediate notes:

 

[*]Ideally, you should try not to mix PHP code and HTML. This may be a little bit too advanced for you at the moment, but you should think about looking into templates (e.g. Smarty templating) if you want to get serious about cleaner code.

[*]choose a naming convention for your variables that doesn't have you sticking words together; it makes things more readable. Instead of $thisismyvariable, use $this_is_my_variable or $thisIsMyVariable

[*]there's almost never a reason to use sleep().

[*]I don't know much about what this code is supposed to do, but I'm almost certain that instead of exec()ing a separate PHP script, you could write a function in a separate file to do what you need, then use include() to pull that file into your script

 

[Pikachu2000]

While you're at it, you may as well replace all of your <? tags with the full <?php tags.

[semperfi]

A couple of immediate notes:

 

[*]Ideally, you should try not to mix PHP code and HTML. This may be a little bit too advanced for you at the moment, but you should think about looking into templates (e.g. Smarty templating) if you want to get serious about cleaner code.

[*]choose a naming convention for your variables that doesn't have you sticking words together; it makes things more readable. Instead of $thisismyvariable, use $this_is_my_variable or $thisIsMyVariable

[*]there's almost never a reason to use sleep().

[*]I don't know much about what this code is supposed to do, but I'm almost certain that instead of exec()ing a separate PHP script, you could write a function in a separate file to do what you need, then use include() to pull that file into your script

 

Thanks for the advice and unfortunately I always go the hard route but I do always seem to pick up things fast with practice and help. I just wanted to see how this code would be if optimized so I can see a side by side comparison from the right to the wrong.

 

and in regards to Pikachu2000 <? tags to <?php my mistake overlooked it and fixed ( thanks ).   

[btherl]

The biggest improvement you could make to that code would be to document it   I have no idea what those 1s and 3s and 4s are doing.  A number that appears in code without any explanation is often called a "magic number", because it appears to work by magic, without helping the reader understand how it works.

 

Full marks for indenting though - your code structure is very easy to read, and I can see the flow of control immediately.

[semperfi]

The biggest improvement you could make to that code would be to document it   I have no idea what those 1s and 3s and 4s are doing.  A number that appears in code without any explanation is often called a "magic number", because it appears to work by magic, without helping the reader understand how it works.

 

Full marks for indenting though - your code structure is very easy to read, and I can see the flow of control immediately.

 

Those numbers are Magical  and yes I do have it documented/commented just stripped it for posting. Thank you I tried to make it easy to read I guess it worked