Efficient if statement / for loop

Question

2 short questions based on trying to make my code more efficient (I think my ultimate quest is to make my entire (fairly complex) website based on some sort of MVC framework, but not being a professional programmer, I think that's going to be a long and steep learning curve..)

In this code, is there a way to merge the if statement and for loop, to avoid the nesting:

if($fileatt['name']!=null)
{
  $attachedFiles = "You uploaded the following file(s)\n";
  for($i=0;$i<count($docNames);$i++)
  {
    $attachedFiles = $attachedFiles. " - " . $docNames[$i] . "\n";
  }
}

At the moment, I do the fairly standard thing of splitting my $_POST array from a form submission, 'clean' the contents and store the elements in individual variables:
```
$name = cleanInput($_POST['name']);
$phone = cleanInput($_POST['phone']);
$message = cleanInput($_POST['message']);
...
```

(where cleanInput() contains striptags() and mysql_real_escape_string())

I had thought that keeping all the information in an array might my code more efficient, but is there a way to apply a function to all (or selected) elements of an array? For example, in R, this is what the apply() function does.

Alternatively, given that all my variables have the same name as in the $_POST array, is there a way to generate all the variables dynamically in a foreach loop? (I know the standard answer when people ask if they can dynamically generate variables is to use a hashmap or similar, but I was interested to see if there's a technique I've missed)

Andreas Wong · Answer 1 · 2012-03-29T10:28:55.117

4

You can use extract and combine it with array_map

extract(array_map('cleanInput', $_POST), EXTR_SKIP);

echo $name; // outputs name

Be warned that $_POST could be anything and user can then submit anything to your server and it becomes a variable in your code, thus if you have things like

if(empty($varName)) { } // assumes $varName is empty initially

Could easily bypassed by user submitting $_POST['varName'] = 1

To avoid mishaps like this, you can have a whitelist of array and filter out only those you need:

$whitelist = array('name', 'phone', 'message');
$fields = array();

foreach($_POST as $k => $v) {
   if(in_array($k, $whitelist)) $fields[$k] = $v;
}

extract(array_map('cleanInput', $fields));

edited Mar 29 '12 at 10:28

answered Mar 29 '12 at 10:16

Andreas Wong

59,630
19
106
123

OMG - you should *always* use a prefix with extract! – symcbean Mar 29 '12 at 10:30
Thanks - this is very interesting. I'll definitely use it in conjunction with the points the other respondants have made – ChrisW Mar 30 '12 at 01:07

score 1 · Answer 2 · answered Mar 29 '12 at 10:15

Personally I think that the performance cost of using an "If" statement is worth the benefit of having easily readable code. Also you have to be sure that you actually use fewer cycles by combining, if there is such a way.

I'm not sure I follow your second question, but have you looked at extract() and array_walk() yet?

score 1 · Accepted Answer · answered Mar 29 '12 at 11:39

1) To the first question, how to merge the if and the for loop:

Why would you want to merge this, it will only make the code more difficult to read. If your code requires an if and afterwards a for loop, then show this fact, there is nothing bad with that. If you want to make the code more readable, then you can write a function, with a fitting name, e.g. listAttachedFiles().

2) To the question about cleaning the user input:

There is a difference between input validation and escaping. It's a good thing to validate the input, e.g. if you expect a number, then only accept numbers as input. But escaping should not be done until you know the target system. So leave the input as it is and before writing to the db use the mysql_real_escape_string() function, before writing to an HTML page use the function htmlspecialchars().

Combining escape functions before needed, can lead to invalid data. It can become impossible to give it out correctly, on a certain target system.

Thanks - I think this is what I thought the answer to 1) would be. The discussion about 2) is interesting; I actually took inspiration from the cleaning function from http://stackoverflow.com/a/544302/889604 - I'd always assumed that it's best to clean input as soon as possible, although you and @symcbean both say that isn't necessarily correct — ChrisW, Mar 30 '12 at 01:06
@ChrisW - Yes, it is tempting to write such a "do it once and forget" function, but you can/will get into hot water. Because it is difficult to never forget to escape, you can write a wrapper around the target system, that takes care of the escaping. PHP's [PDO](http://www.php.net/manual/en/book.pdo.php) library is such a wrapper for database access. — martinstoeckli, Mar 30 '12 at 06:52

score 0 · Answer 4 · answered Apr 12 '12 at 22:43

To make your for loop more efficient don't use Count() within the condition of your loops.

It's the first thing they teach in school. As the For loops are reevaluating the conditions at each iterations.

$nbOfDocs = count($docNames); //will be much faster
for($i=0;$i<$nbOfDocs;$i++)
{
   $attachedFiles = $attachedFiles. " - " . $docNames[$i] . "\n";
}

score 0 · Answer 5 · answered Mar 29 '12 at 10:28

Point 1 is premature optimization. And you want get any better performance / readability by doing so. (similar for using arrays for everything).

Point 2 - AaaarrgghhH! You should only change the representation of data at the point where it leaves PHP, using a method approporiate to the destination - not where it arrives in PHP.

Efficient if statement / for loop

5 Answers5