Solved

Please help me refactor this PHP code, it works properly but it seems inefficient as heck

Posted on 2014-01-06
10
290 Views
Last Modified: 2014-01-06
Please help me refactor this PHP code, it works properly but it seems inefficient as heck

 <?php
		
		
		if ($access_level == "") {
			echo '<option value="user"> User</option>';
			echo '<option value="administrator"> Administrator</option>'; 
		} elseif ($access_level == "administrator") {
			echo '<option value="administrator" selected = "selected"> Administrator</option>'; 
			echo '<option value="user"> User</option>';
		} elseif ($access_level == "user") {
			echo '<option value="administrator" > Administrator</option>'; 
			echo '<option value="user" selected = "selected"> User</option>';
		} else {
			echo '<option value="user"> last one</option>';
			echo '<option value="administrator" > Administrator</option>'; 
		}


?>

Open in new window

0
Comment
Question by:LB1234
  • 4
  • 3
  • 2
  • +1
10 Comments
 
LVL 108

Expert Comment

by:Ray Paseur
ID: 39759272
It looks just fine to me.  You can tinker with it, but there is really no efficiency to be gained.  And if you doubt that, attach a script timer to the before and after versions.  I doubt if you will find a microsecond!

To me the greater question is "can you trust the external variables?" and we do not see where they come from here, so the answer has to be "nope."  I believe (without testing) that no matter what the external variables contain, the script will generate something that makes sense.  Of course, I could be wrong if the external variable contained an 80GB object, or a floating point number or something goofy. :-)
0
 
LVL 1

Author Comment

by:LB1234
ID: 39759306
<?php
        if(isset($_POST["submit"])) {
	  	$first_name = $_POST["first_name"];
	  	$last_name = $_POST["last_name"];
	  	$user_name = $first_name . "_" . $last_name;
	  	$access_level = $_POST["access_level"];
		$department = $_POST["department"];
		$password = $_POST["password"];
	  	$password2 = $_POST["password2"];
		 	
		  if ($password !== $password2) {
			echo "Passwords do not match, please re-enter";  
		  } else {
			redirect_to("account_created.php");
		  }
	} else {
  
	  	$first_name = "";
	  	$last_name = "";
	  	$user_name = "";
	  	$access_level = "";
		$department = "";
		$password = "";
	  	$password2 = "";
	  	$message = "Please log in";
	}

Open in new window

0
 
LVL 1

Author Comment

by:LB1234
ID: 39759313
But Ray that seems like a lot of code for just three possible outcomes. What if there were many more options.  My gut is telling me that there's a way to reduce the number of lines of code.  I was thinking ternary but I don't know enough about the syntax to pull that off. :(
0
 
LVL 34

Assisted Solution

by:Dan Craciun
Dan Craciun earned 25 total points
ID: 39759337
Why don't you use a switch?

Here's an attempt, not reducing the number of lines, but reducing the repetitive typing:

<?php

$optionUser  = '<option value="user"';
$optionAdmin = '<option value="administrator"';
$userName = " User";
$adminName = " Administrator";
$selected = ' selected = "selected"';

switch ($access_level) {
	case "":
		break;
	case "administrator":
		$optionAdmin .= $selected;
		break;
	case "user":
		$optionUser .= $selected;
		break;
	default:
		$userName = " last one";
	
}
$optionUser  .= '>' . $userName . '</option>';
$optionAdmin .= '>' . $adminName . '</option>';

echo $optionUser;
echo $optionAdmin;

?>

Open in new window

0
 
LVL 34

Accepted Solution

by:
gr8gonzo earned 450 total points
ID: 39759347
You're probably thinking of:

if( expression ? value when true : value when false), like this:

echo ( 1 > 0 ? "One is always greater than zero!" : "Your processor or PHP build has some math problems.");

If you're going to add several options, try this approach:
<?php
// Define the options and their values
$options = array(
  "user" => "User",
  "admin" => "Administrator",
  "superadmin" => "Super Administrator",
  "superduperadmin" => "Etc"
);

// Build the HTML
$options_html = "";
foreach($options as $option_val => $option_text)
{
  $selected = ($access_level == $option_val ? "selected" : "");
  $options_html .= "<option value='{$option_val}'>{$option_text}</option>\n";
}

// Display the HTML
echo $options_html;

Open in new window

0
Is Your Active Directory as Secure as You Think?

More than 75% of all records are compromised because of the loss or theft of a privileged credential. Experts have been exploring Active Directory infrastructure to identify key threats and establish best practices for keeping data safe. Attend this month’s webinar to learn more.

 
LVL 108

Assisted Solution

by:Ray Paseur
Ray Paseur earned 25 total points
ID: 39759365
The $access_level is an external variable in the POST request and is by definition tainted.  The sort of question this raises for me is, "Can anyone be both a user and an administrator?"  If so, how would you account for that in the HTML <select>?  Maybe use multiple?  And if nothing is selected...  what's the default?

When you get to very, very long lists of possibilities, there are things like arrays of objects that make sense, but here the simple if() statement seems OK.  You can't spend too much time on this stuff before the ROI goes negative.  http://xkcd.com/1205/
0
 
LVL 1

Author Comment

by:LB1234
ID: 39759378
Gr8, that is EXACTLY what i was looking for, something which doesn't need an additional line of code for each new option: just an addition to the array/pool of choices!

Ray and others, thank you.
0
 
LVL 34

Expert Comment

by:gr8gonzo
ID: 39759402
I made a mistake in my code - I forgot to use the $selected variable! Replace line 15 with:

$options_html .= "<option value='{$option_val}' {$selected}>{$option_text}</option>\n";
0
 
LVL 1

Author Comment

by:LB1234
ID: 39759406
Gr8, understanding that code seems like a turning point in my life as a coder.  I'm going to spend the next 30 mins going over the logic of that until I get it.
0
 
LVL 108

Expert Comment

by:Ray Paseur
ID: 39759410
Try a couple of these, where we use a GET-method request to make testing easy:
http://www.laprbass.com/RAY_temp_lb1234.php?a=user
http://www.laprbass.com/RAY_temp_lb1234.php?a=foo
http://www.laprbass.com/RAY_temp_lb1234.php

Relevant man pages:
http://php.net/ternary
http://php.net/manual/en/language.types.string.php#language.types.string.syntax.heredoc


<?php // RAY_temp_lb1234.php
error_reporting(E_ALL);

// USE TERNARY OPERATOR TO GET ACCESS LEVEL FROM REQUEST
$access_level = !empty($_GET['a']) ? (string)$_GET['a'] : NULL;

// SET DEFAULT VALUES TO NULL
$oa = $ou = NULL;

// CHOOSE AMONG COMPETING OPTIONS
switch (strtolower(trim($access_level)))
{
    case "administrator" : $oa = ' selected '; break;
    case "user"          : $ou = ' selected '; break;
}

// USE HEREDOC TEMPLATE TO CREATE HTML FRAGMENT
$html = <<<EOD
<option value="user"          $ou> User         </option>
<option value="administrator" $oa> Administrator</option>
EOD;

// SHOW THE WORK PRODUCT
echo htmlentities($html);

Open in new window

0

Featured Post

Is Your Active Directory as Secure as You Think?

More than 75% of all records are compromised because of the loss or theft of a privileged credential. Experts have been exploring Active Directory infrastructure to identify key threats and establish best practices for keeping data safe. Attend this month’s webinar to learn more.

Question has a verified solution.

If you are experiencing a similar issue, please ask a related question

Consider the following scenario: You are working on a website and make something great - something that lets the server work with information submitted by your users. This could be anything, from a simple guestbook to a e-Money solution. But what…
Foreword (July, 2015) Since I first wrote this article, years ago, a great many more people have begun using the internet.  They are coming online from every part of the globe, learning, reading, shopping and spending money at an ever-increasing ra…
The viewer will learn how to create and use a small PHP class to apply a watermark to an image. This video shows the viewer the setup for the PHP watermark as well as important coding language. Continue to Part 2 to learn the core code used in creat…
This tutorial will teach you the core code needed to finalize the addition of a watermark to your image. The viewer will use a small PHP class to learn and create a watermark.

943 members asked questions and received personalized solutions in the past 7 days.

Join the community of 500,000 technology professionals and ask your questions.

Join & Ask a Question

Need Help in Real-Time?

Connect with top rated Experts

15 Experts available now in Live!

Get 1:1 Help Now