asked on

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

Ray Paseur

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. :-)

LB1234

ASKER

<?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

LB1234

ASKER

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. :(

SOLUTION

Dan Craciun

membership

This solution is only available to members.

To access this solution, you must be a member of Experts Exchange.

Start Free Trial

ASKER CERTIFIED SOLUTION

gr8gonzo

membership

This solution is only available to members.

To access this solution, you must be a member of Experts Exchange.

Start Free Trial

SOLUTION

Ray Paseur

membership

This solution is only available to members.

To access this solution, you must be a member of Experts Exchange.

Start Free Trial

LB1234

ASKER

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.

gr8gonzo

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";

LB1234

ASKER

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.

Ray Paseur

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