2
\$\begingroup\$

Part of an application that I am building in CakePHP 2.1 needs to send out an e-mail based on a template from a database. The end user is able to set the from, reply to, to, cc, and bcc addresses. Because the user could enter just about anything, I need to process those addresses into a usable array that I can pass to CakeEmail. To this end, I wrote the following code and I want to make sure that it is efficient before I put it into production use (and possibly write a tutorial about it).

<?php
$parseAddress = function(&$address) {
    preg_match_all('/"(.*?)"\s*\<(.*?)\>/', $address, $matches); // Check if $address is in the format ""Test Address" <test@example.com>". TODO: Make the quotes optional.

    if (!is_array($matches) || !isset($matches[1]) || empty($matches[1]) || !isset($matches[2]) || empty($matches[2]) || is_null(array_filter($matches))) {
        return $address; // Return the plain e-mail address if name is not provided
    }

    $parsedAddress = array();

    for ($i = 0; $i < count($matches[0]); $i++) {
        $parsedAddress[$matches[2][$i]] = $matches[1][$i]; // Add e-mail address to array as "email => name"
    }

    $addressCSV = str_getcsv($address); // Split e-mail addresses into array - seems to automatically strip anything between "<" and ">" characters
    foreach($addressCSV as $key => $val) {
        if (!filter_var($val, FILTER_VALIDATE_EMAIL)) { // Take advantage of afformentioned stripping by filtering out everything that is not an e-mail address
            unset($addressCSV[$key]); // Remove the array value that is not an e-mail address
        }
    }

    return array_merge($addressCSV, $parsedAddress); // Join the parsed e-mail addresses with the filtered addresses and return the resulting array
};

echo $parseAddress('test@address.tld'); // returns (string) "test@address.tld"
echo $parseAddress('"Test Address" <test@example.com>, test@address.tld, "Smith, John"<jsmith@example.com>'); // returns (array) [0 => "test@address.tld", "test@example.com" => "Test Address", "jsmith@example.com" => "Smith, John"]
\$\endgroup\$
  • \$\begingroup\$ Our team was kicked off the project due to budget concerns before I got the chance to refactor this code. Oh well. \$\endgroup\$ – Joseph Leedy Aug 5 '12 at 17:19
2
\$\begingroup\$

Alright, I have a number of suggestions. Some of which will invalidate others. The reason I'm keeping these "invalid" bits in is because they are just good practice and I'm trying to pass that along. In order to get the most from this you will have to read all the way through.

First off, why are you using lambda functions (also known as anonymous functions or closures)? This seems completely unnecessary. The thing to keep in mind about lambda functions is that they are not compiled at run time. This means that they should be used sparingly because each time they are called they must be compiled separately. Lambda functions should really only be used for callbacks. Though I'll be honest, I don't even use them then. They tend to make the functions they are used in bulky and therefore difficult to read. In the five, almost six, years I've been doing this I've never once used, or needed, lambda functionality. Best to just use a real function and set a return value. Though that may just be an opinion.

REGEX should only be used as a last resort. Yes it is much more powerful than any of the simpler string functions, but because of that power it requires more processing power as well, and is therefore usually slower. So, if you can do the same thing with a simpler function, it is more than likely better. For instance, you could use explode() here to separate the initial string by spaces to get the first name, last name, and email addresses. If multiple addresses are used you can use explode()'s limiter to only return 3 elements, then use explode() again on the email section. This appears to be what you are doing anyways and is more than likely quicker. Though you may not notice a difference unless you are doing it thousands of times. Besides, REGEX is almost impossible to read and if not documented properly just looks like a foreign language to those of us who come after you and try to figure out what it does. REGEX is another one of those things I've hardly ever needed to have, there is almost always another way.

$matches = explode( ' ', $address, 3 );
list( $firstName, $lastName, $addresses ) = $matches;//for illustration purposes only
$addresses = trim( $addresses, '<>' );//remove those brackets before exploding
$addresses = explode( ', ', $addresses );//could use str_getcsv() here if you wanted

Because explode() always creates an array unless the string passed to it was empty, you can now just check that the result isn't FALSE and then check the array's size to determine if the only thing it contains is an address. This looks much better than that long list of confusing if arguments.

if( $matches === FALSE ) {
    return FALSE;
}
if( count( $matches ) == 1 ) {
    return $address;
}

By the way, you should still validate and sanitize the address with filter_var() in the above scenario. Validate with FILTER_VALIDATE_EMAIL and sanitize with FILTER_SANITIZE_EMAIL. I believe you can even do both at once. Well, I know you can, but I'm not sure if it will return FALSE if its not valid, so you may have to play around with it.

$email = filter_var( $address, FILTER_VALIDATE_EMAIL | FILTER_SANITIZE_EMAIL );

For and while loops declare the functions passed to them as parameters on each iteration, so using count() as a parameter here is inefficient. This value isn't going to change so it should be declared before the loop. Foreach is the only loop that doesn't have this issue, though it is still better, in my opinion, to separate functions from foreach loops as well.

$length = count( $matches[ 0 ] );
for( $i = 0; $i < $length; $i++ ) {

Although, I believe you could get away with just using array_combine() here instead of a loop.

$parsedAddress = array_combine( $matches[ 2 ], $matches [ 1 ] );

str_getcsv() may do what you want, but only because it is a hack. CSV stands for "Comma Separated Values" and is used to store and read data separated by commas. I think, though I can't find it documented anywhere, that I remember reading somewhere that it also uses angle brackets "<>" to separate groups and that is why this works, though I'm not sure. I'm honestly not sure why this works, but you should not rely on hacks to get a job done. What if that function changes? Then if you ever upgrade your server you will be stuck trying to figure out how to fix it, or someone else with less knowledge of how this works will. Besides, you've already gotten those email addresses into an array, why do it again? Just use array_keys() on $parsedAddress to get an array of email addresses. Better yet, while you are looping over $matches the first time and setting them, why not just use filter_var() there? Or if you end up using the array_combine() example, you could use array_map() to apply a callback function, in this case filter_var(), to the array_keys(). You've not just doubled the work here, but quadrupled it. You loop over $matches, then you loop over $addressCSV, then when you merge them, PHP loops over both arrays again to find the differences.

Last thing. Why are you referencing $address? I can find no instance where you are changing its value. This is also unnecessary, and, in my opinion, is a big no-no. Variable referencing is a powerful tool, but, like most powerful tools, it does have some drawbacks. The biggest one is that they are difficult to spot and therefore make troubleshooting difficult. Another is that they aren't well known. So while I can spot this, someone else may have never seen it before. After all, it is just an ampersand and not an actual function that they can easily look up.

I hope this all helps. If you need any clarification, just ask.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks for your feedback @showerhead! Here is my response/resoning: (1) I am using a lambda because this being called from a class method and is only used in that method. (2) I used a regular expression because that is what I know and it seemed the simplest way. (3) How much faster (or slower) would using explode be? (4) I am not too concerned with validation since (a) a limited set of users will enter this data and (b) CakePHP handles this automatically. (5) Never thought about this - I have always done it that way. (6) Yeah I know it is a hack - a dirty one \$\endgroup\$ – Joseph Leedy Jul 31 '12 at 17:21
  • \$\begingroup\$ (7) I referenced address because originally it was being modified before I refactored that part of the code out. I have since removed that which the provided code does not reflect. I will take your suggestions into consideration and maybe use them to further refactor this code. Thanks again! \$\endgroup\$ – Joseph Leedy Jul 31 '12 at 17:25
  • \$\begingroup\$ @Joseph: Looks like it could be made into a private method with the same results, but again, this might be preference. Just keep in mind they shouldn't be used repeatedly as this defeats their purpose. I can't tell you for sure how much difference explode() would make. You can use microtime() to test it. I do know that REGEX functions tend to run slower. However, if you are doing multiple things that is impossible with string functions or very much more complicated, then REGEX is an acceptable alternative. Just document what its doing, not everyone agrees that REGEX is simple ;) \$\endgroup\$ – mseancole Jul 31 '12 at 18:30
  • \$\begingroup\$ I'm marking this as accepted since nobody else chimed in. \$\endgroup\$ – Joseph Leedy Aug 5 '12 at 17:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.