Solved

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

Posted on 2014-01-06
10
298 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 110

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
Technology Partners: We Want Your Opinion!

We value your feedback.

Take our survey and automatically be enter to win anyone of the following:
Yeti Cooler, Amazon eGift Card, and Movie eGift Card!

 
LVL 35

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 35

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
 
LVL 110

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 35

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 110

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

Technology Partners: We Want Your Opinion!

We value your feedback.

Take our survey and automatically be enter to win anyone of the following:
Yeti Cooler, Amazon eGift Card, and Movie eGift Card!

Question has a verified solution.

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

Introduction HTML checkboxes provide the perfect way for a web developer to receive client input when the client's options might be none, one or many.  But the PHP code for processing the checkboxes can be confusing at first.  What if a checkbox is…
Password hashing is better than message digests or encryption, and you should be using it instead of message digests or encryption.  Find out why and how in this article, which supplements the original article on PHP Client Registration, Login, Logo…
The viewer will learn how to count occurrences of each item in an array.
The viewer will learn how to look for a specific file type in a local or remote server directory using PHP.

679 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