Solved

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

Posted on 2014-01-06
10
288 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
How your wiki can always stay up-to-date

Quip doubles as a “living” wiki and a project management tool that evolves with your organization. As you finish projects in Quip, the work remains, easily accessible to all team members, new and old.
- Increase transparency
- Onboard new hires faster
- Access from mobile/offline

 
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

Maximize Your Threat Intelligence Reporting

Reporting is one of the most important and least talked about aspects of a world-class threat intelligence program. Here’s how to do it right.

Join & Write a Comment

This article will explain how to display the first page of your Microsoft Word documents (e.g. .doc, .docx, etc...) as images in a web page programatically. I have scoured the web on a way to do this unsuccessfully. The goal is to produce something …
I imagine that there are some, like me, who require a way of getting currency exchange rates for implementation in web project from time to time, so I thought I would share a solution that I have developed for this purpose. It turns out that Yaho…
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.

757 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

25 Experts available now in Live!

Get 1:1 Help Now