3

I have the following function.
I can run it to test if true, or else false or vice versa as shown.

    $fname=$_POST['fname'];
    $lname=$_POST['lname'];

function namecheck ($fname,$lname) 
{    
    $names= array ($fname,$lname);

    $regexp ="/^[A-Za-z]+$/";

    //filter through names 
    for ($i=0; $i<2; $i++)
    if (! preg_match($regexp, $names[$i])) 
    {
         return false;
    }
    return true;
}

(Alternate Version):

for ($i=0; $i<2; $i++)
if (preg_match($regexp, $names[$i])) 
{
    return true;
}
return false;

Which is the better way to write this, both in terms of efficiency and good coding practices? Or is there no difference?

It may not be an issue for a small array like this, but I am wondering what effect it would have on a larger and more complex program.

Cody Gray - on strike
  • 239,200
  • 50
  • 490
  • 574

3 Answers3

9

The difference is that both the loops are checking for different results.

  • The 1st loop is checking whether the $regexp matches all the elements of the array - In this case, it returns false as soon as a match fails and if the return statement after the for loop is reached, that means all the elements match.
    To be honest, not having the braces around the for loop can often be confusing (like it confused me first). I would suggest to add relevant brace:

    for ($i=0; $i<2; $i++) {
    if (preg_match($regexp, $names[$i])) {
         return true;
    }
}
return false;

  • The 2nd loop is checking whether the $regexp matches any element of the array - In this case, it return true as soon as a match succeeds, and if the return statement after the for loop is reached, that means none of the elements match. 

    for ($i=0; $i<2; $i++) {
    if (! preg_match($regexp, $names[$i])) {
         return false;
    }
}
return true;
Rohit Jain
  • 209,639
  • 45
  • 409
  • 525
  • @Skippy. Check out your loops. In both the cases, if `if` condition is evaluated to `true`, if will return the value from `if` block, and if the condition is false, it won't go inside the `if` block, and execute the last statement in for loop, which will return something else. In any case, your loop will only iterate once. – Rohit Jain Aug 10 '13 at 08:36
  • 1
    @Skippy. Oh! great. I didn't see that you don't have curly braces around the `for` loop. I mistakenly took the closing braces of method, as closing braces of `for` loop. – Rohit Jain Aug 10 '13 at 08:39
  • 1
    @Skippy. That's why you should never remove the braces from the loop. It will only confuse people. – Rohit Jain Aug 10 '13 at 08:40
  • 1
    RohitJain nicely done +1 ... @Skippy there is many bad practice in your code in your code .. i see no reason to not validate input you should first check weather $_POST is set your not – NullPoiиteя Aug 10 '13 at 08:46
  • @Skippy thankyou .. check this http://stackoverflow.com/a/8013972/1723893 if you are targeting international people name than name can have words other than a-z and about the function and loop i am agree with Rohit .. so adding another answer wouldnt be worthy :) – NullPoiиteя Aug 10 '13 at 08:58
4

In your code you would check only one iteration ($i = 0)

You could use:

for ($i=0; $i<2; $i++){
    if (!preg_match($regexp, $names[$i])) return false;
}
return true;

If there would be error method would return false, if no -> true. Method would stop iteration on first error.

Valentin Logvinskiy
  • 121
  • 4
4

I'd recommend to give your function a name that makes more sense, like is_name_valid(). It helps clarity and your "problem" will instantly go away since it will have to return True when the name satisfies your requirements or False otherwise.

3k-
  • 2,467
  • 2
  • 23
  • 24
  • I actually use camelCase myself, I only wanted to follow the style of the code in the question ("namecheck"). – 3k- Aug 10 '13 at 08:42