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

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

LVL 1
LB1234Asked:
Who is Participating?
 
gr8gonzoConnect With a Mentor ConsultantCommented:
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
 
Ray PaseurCommented:
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
 
LB1234Author Commented:
<?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
Get your problem seen by more experts

Be seen. Boost your question’s priority for more expert views and faster solutions

 
LB1234Author Commented:
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
 
Dan CraciunConnect With a Mentor IT ConsultantCommented:
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
 
Ray PaseurConnect With a Mentor Commented:
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
 
LB1234Author Commented:
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
 
gr8gonzoConsultantCommented:
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
 
LB1234Author Commented:
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
 
Ray PaseurCommented:
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
All Courses

From novice to tech pro — start learning today.