Returning variables from a function

starsol · September 11th, 2003, 07:29 AM

I'm adding a feature to a script that allows people to block the script from ever sending them an email - and it works fine.

The code to check if an email address is blocked is as follows:

Quote:

quote:
$blocked_file = "members/block_list_file.txt";

$file = fopen($blocked_file,"r");
$list = fread($file, filesize($blocked_file));
fclose($file);
$list=stripslashes($list);
$out="";
$lines = explode("|",$list);
foreach($lines as $l){
        if ($l != $email){
        $out .= "|".$l;
        }
        else {
        $is_blocked = "1";
        }
    }

However, when I try to use it as a function:

Quote:

quote:
function blockcheck($email){

$blocked_file = "members/block_list_file.txt";

$file = fopen($blocked_file,"r");
$list = fread($file, filesize($blocked_file));
fclose($file);
$list=stripslashes($list);
$out="";
$lines = explode("|",$list);
foreach($lines as $l){
        if ($l != $email){
        $out .= "|".$l;
        }
        else {
        $is_blocked = "1";
        }
    }

return $is_blocked;

}

and call it:

Quote:

quote:
blockcheck($email)

$is_blocked is never 1. So I assume that the "return $is_blocked;" is incorrect. I've checked lots of places, and returning variables from functions is always written the same, but for some reason it is not valid in this case. Can anyone give me a pointer as to what I'm doing wrong?

Thanks and very best wishes,
Rupe

Rupe Parnell
www.starsol.co.uk

nikolai · September 11th, 2003, 12:58 PM

Okay, this might be stupid questions, but here goes:

Where is $is_blocked NOT 1?
Where and how are you checking it?
What does your function DO, exactly?
Why do you build up the $out string if you're not going to use it?
Why do you set the value of $is_blocked to the string "1" instead of the integer 1, or better yet, the boolean true?

See, your function answers a question: Is this email blocked? That answer is yes or no; boolean values.

Another problem -- you search through the ENTIRE file contents before returning a value. If your blocked email is the first in the list, you'll still waste the time making string comparisons for every other address in the array... why?

Here's a much more efficient rewriting of your function:

function is_blocked($email)
{
    $blocked_file = "members/block_list_file.txt";

    $list = file_get_contents($blocked_file);
    $list = stripslashes($list);

    $lines = explode("|",$list);

    return in_array($email, $lines);
}

http://www.php.net/file_get_contents
http://www.php.net/in_array

See, you pass in an email address. The function tells you if that email address is in the file. Here's another rewriting:

function is_blocked($email)
{
    $blocked_file = "members/block_list_file.txt";

    $list = file_get_contents($blocked_file);
    $list = stripslashes($list);

    return (false !== stripos("|{$email}|", $list));
}

http://www.php.net/stripos

This rewriting is nicer because you don't spend a bunch of time or waste memory splitting the file contents up into an array.

Take care,

Nik
http://www.bigaction.org/