Go Premium for a chance to win a PS4. Enter to Win

x
?
Solved

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

Posted on 2014-01-06
10
Medium Priority
?
310 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 111

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
Concerto's Cloud Advisory Services

Want to avoid the missteps to gaining all the benefits of the cloud? Learn more about the different assessment options from our Cloud Advisory team.

 
LVL 35

Assisted Solution

by:Dan Craciun
Dan Craciun earned 100 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 1800 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 111

Assisted Solution

by:Ray Paseur
Ray Paseur earned 100 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 111

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

VIDEO: THE CONCERTO CLOUD FOR HEALTHCARE

Modern healthcare requires a modern cloud. View this brief video to understand how the Concerto Cloud for Healthcare can help your organization.

Question has a verified solution.

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

This article discusses how to create an extensible mechanism for linked drop downs.
Many old projects have bad code, but the budget doesn't exist to rewrite the codebase. You can update this code to be safer by introducing contemporary input validation, sanitation, and safer database queries.
The viewer will learn how to dynamically set the form action using jQuery.
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…
Suggested Courses

772 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