views:

134

answers:

7

Can someone please help me simpling this redundant piece of code?

if (isset($to) === true)
{
    if (is_string($to) === true)
    {
        $to = explode(',', $to);
    }

    $to = array_filter(filter_var_array(preg_replace('~[<>]|%0[ab]|[[:cntrl:]]~i', '', $to), FILTER_VALIDATE_EMAIL));
}

if (isset($cc) === true)
{
    if (is_string($cc) === true)
    {
        $cc = explode(',', $cc);
    }

    $cc = array_filter(filter_var_array(preg_replace('~[<>]|%0[ab]|[[:cntrl:]]~i', '', $cc), FILTER_VALIDATE_EMAIL));
}

if (isset($bcc) === true)
{
    if (is_string($bcc) === true)
    {
        $bcc = explode(',', $bcc);
    }

    $bcc = array_filter(filter_var_array(preg_replace('~[<>]|%0[ab]|[[:cntrl:]]~i', '', $bcc), FILTER_VALIDATE_EMAIL));
}

if (isset($from) === true)
{
    if (is_string($from) === true)
    {
        $from = explode(',', $from);
    }

    $from = array_filter(filter_var_array(preg_replace('~[<>]|%0[ab]|[[:cntrl:]]~i', '', $from), FILTER_VALIDATE_EMAIL));
}

I tried using variable variables but without success (it's been a long time since I've used them).

+1  A: 

I could do this:

You can probably create a function for that:

function checkIt($var)
{
    if (isset($var) === true)
    {
        if (is_string($var) === true)
        {
          $var = explode(',', $var);
        }

        $to = explode(',', $var);
        $to = array_filter(filter_var_array(preg_replace('~[<>]|%0[ab]|[[:cntrl:]]~i', '', $to), FILTER_VALIDATE_EMAIL));
    }

    return $to;
}

And now you can pass your variables to this function.

Web Logic
But then it won't filter if the variable isn't a string, as in the OP.
adam
@adam: But there is no `else` condition that if it is not a string process it otherwise. He is only interested in string so the code executes only if it is string not otherwise.
Web Logic
Because there's no else condition, it explodes it if it's a string but filters it irrespective of whether it's a string or not. Have another look.
adam
@adam: Rightly spotted, fixed :)
Web Logic
The `checkIt` function returns an undefined value `$to` in the case where $var is set but isn't a string.
David Gelhar
@David: It won't, it is assigned to `$to` at line: `$to = explode(',', $var);`
Web Logic
+2  A: 

You could do (untested)

$vars = array($from, $to, $cc, $bcc);

foreach ($vars as $var)
        {
        $var = explode(',', $var);
        ....
        ...
        }

$from = $vars[0];
$to = $vars[1];
$cc = $vars[2];
$bcc = $vars[3];
nico
Eric
+2  A: 

Put it in a function?

function validate($str) {
    if (isset($str) === true)
    {
        if (is_string($str) === true)
        {
            $str = explode(',', $str);
        }

        $str = array_filter(filter_var_array(preg_replace('~[<>]|%0[ab]|[[:cntrl:]]~i', '', $str), FILTER_VALIDATE_EMAIL));
    }
    return $str;
}

$to = validate($to);
$cc = validate($cc);
$bcc = validate($bcc);
$from = validate($from);
adam
+1  A: 

Just stick the values in an array and iterate over it.

function cleanEmails($value) {
    if (is_string($value)) {
        $value = explode(',', $value);
    }
    return array_filter(filter_var_array(preg_replace('~[<>]|%0[ab]|[[:cntrl:]]~i', '', $value), FILTER_VALIDATE_EMAIL));
}

$fields = array();
if (isset($to)) {
    $fields['to'] = $to;
}
if (isset($from)) {
    $fields['from'] = $from;
}
if (isset($cc)) {
    $fields['cc'] = $cc;
}
if (isset($bcc)) {
    $fields['bcc'] = $bcc;
}
$result = array_map('cleanEmails', $fields);

The end result will be a 2 dimensional array, first index will be the fields that were set, second index will be the respective email addresses...

wimvds
+1  A: 

Even without going the full variable variables route, you could simplify this a lot just by putting the checks into a common function, and then doing:

$to = cleanup_email_addrs($to);
$cc = cleanup_email_addrs($cc);
$bcc = cleanup_email_addrs($bcc);
$from = cleanup_email_addrs($from);
David Gelhar
+1  A: 

For starters you can get rid of the isset() === true; isset() returns either true or false.

And of course put it in a function as all your if statements seem to do the same thing, but that´s been mentioned before...

By the way, does your array_filter line work if the input is not an array()?

If not, you need to include that statement inside the if (is_string()) statement.

jeroen
+5  A: 

Variable variables:

$vars = array('to', 'cc', 'bcc', 'from');
foreach ($vars as $varname) {
    if (isset($$varname)) {
        if (is_string($$varname)) {
            $$varname = explode(',', $$varname);
        }
        $$varname = array_filter(filter_var_array(preg_replace('~[<>]|%0[ab]|[[:cntrl:]]~i', '', $$varname), FILTER_VALIDATE_EMAIL));
    }
}

Regular (Without using variable variables):

$vars = compact('to', 'cc', 'bcc', 'from'); 
foreach ($vars as $name => &$var) {
    if (is_string($var)) {
        $var = explode(',', $var);
    }
    $var = array_filter(filter_var_array(preg_replace('~[<>]|%0[ab]|[[:cntrl:]]~i', '', $var), FILTER_VALIDATE_EMAIL));
}
extract ($vars);

Note, you don't need isset, because compact will only import variables that are set. All others are ignored...

BTW: You don't need the === true. isset() or is_string() will always return a boolean. So the === true is redundant...

ircmaxell
`compact()`... Brilliant! Thank you. :)
Alix Axel
@ircmaxell: I'd just like to add that it's necessary to do `compact($to, $cc, $bcc, $from)` for it to skip null variables, otherwise we still need the `isset()` check.
Alix Axel