Can this be optimized for faster performance and high load?

[acctman]

can the coding I wrote below be optimized in anyway to improve the performance? Does my sql query look good?

 

<?php
session_start();
$userid = $_SESSION['userid'];

if ($_GET['sec'] != 3 && $userid == '') {
header("Location: http://www.sitemain.com");
exit;
}

ob_start(); //hold output
require_once '../password.php';

if (isset($_GET['mid']) && isset($_GET['iid']) && isset($_GET['idat']) && isset($_GET['sec'])) {

$mid = $_GET['mid'];
$iid = $_GET['iid'];
$date = $_GET['idat'];

	if ($_GET['sec'] == 1) { $type = "imgPvt";
	$allowed = 0;
		if (isset($_SESSION['userid']) && $_SESSION['userid'] > 0) {
			$sql = "select p_id from rate_private where p_from='$mid' and p_to='" .$_SESSION['userid']. "'";
			$result = mysql_query($sql);
			if (mysql_num_rows($result) > 0 || $mid == $_SESSION['userid'] || $_SESSION['userid'] == '39') {
			$allowed = 1;
			} elseif ($allowed == 0 && $_GET['sec'] == 1) {
				header("Location: http://www.sitemain.com");
				exit;
			}
		} 
	}	

if ($_GET['sec'] == 2) { $type = "imgExp"; } 
if ($_GET['sec'] == 3) { $type = "imgPub"; }

header("Content-type: image/jpeg");
if ($_GET['tmb'] == 1) {				
  	$im = @imagecreatefromjpeg('http://www.site.com/'.$date.'/'.$mid.'/'.$type.'/imgTmb/'.$mid.'-'.$iid.'.jpg');
  	if(!$im) {
  		exit();
  	}
} else { 
	$im = @imagecreatefromjpeg('http://www.site.com/'.$date.'/'.$mid.'/'.$type.'/'.$mid.'-'.$iid.'.jpg');				    
  	if(!$im) {
  		exit();
	  }
}			
imagejpeg($im);
imagedestroy($im);					
}

$buf = ob_get_contents();
ob_end_clean();
print $buf;
?>

[Yesideez]

I've changed two things - merged the SQL stuff into one line and replaced a large if() chunk with a ternary taking about 8 or 9 lines town to 4

 

<?php
session_start();
$userid = $_SESSION['userid'];

if ($_GET['sec'] != 3 && $userid == '') {
  header("Location: http://www.sitemain.com");
  exit;
}

ob_start(); //hold output
require_once '../password.php';

if (isset($_GET['mid']) && isset($_GET['iid']) && isset($_GET['idat']) && isset($_GET['sec'])) {

  $mid = $_GET['mid'];
  $iid = $_GET['iid'];
  $date = $_GET['idat'];

  if ($_GET['sec'] == 1) {
    $type = "imgPvt";
    $allowed = 0;
    if (isset($_SESSION['userid']) && $_SESSION['userid'] > 0) {
      if (mysql_num_rows(mysql_query("select p_id from rate_private where p_from='$mid' and p_to='" .$_SESSION['userid']. "'")) > 0 || $mid == $_SESSION['userid'] || $_SESSION['userid'] == '39') {
        $allowed = 1;
      } elseif ($allowed == 0 && $_GET['sec'] == 1) {
        header("Location: http://www.sitemain.com");
        exit;
      }
    } 
  }
  if ($_GET['sec'] == 2) { $type = "imgExp"; } 
  if ($_GET['sec'] == 3) { $type = "imgPub"; }
  header("Content-type: image/jpeg");
  $im = @imagecreatefromjpeg('http://www.site.com/'.$date.'/'.$mid.'/'.$type.'/'.($_GET['tmp']==1 ? 'imgTmb/' : '').$mid.'-'.$iid.'.jpg');
  if(!$im) {
    exit();
  }
  imagejpeg($im);
  imagedestroy($im);
}

$buf = ob_get_contents();
ob_end_clean();
print $buf;
?>

[Yesideez]

Oh, on any $_GET you're expecting a number I'd use intval() just to make sure you get a number and not anything nasty. Anything other than a number will return 0.

 

The only other thing I'd change is this:

    if (isset($_SESSION['userid']) && $_SESSION['userid'] > 0) {

to this:

    if (intval($_SESSION['userid']) > 0) {

 

Why check twice when you know the contents need to be greater than 0? Anything non-numeric and intval() will return 0 anyway.

[acctman]

Oh, on any $_GET you're expecting a number I'd use intval() just to make sure you get a number and not anything nasty. Anything other than a number will return 0.

 

The only other thing I'd change is this:

    if (isset($_SESSION['userid']) && $_SESSION['userid'] > 0) {

to this:

    if (intval($_SESSION['userid']) > 0) {

 

Why check twice when you know the contents need to be greater than 0? Anything non-numeric and intval() will return 0 anyway.

 

thanks for the tips and the improved coding. I've always wondered about this line...

if (isset($_GET['mid']) && isset($_GET['iid']) && isset($_GET['idat']) && isset($_GET['sec'])) {

 

does that look good?