Today I needed to write some code to grab a list of CC addresses from an email, and I thought showing the steps I took could make for an entertaining blog post.
I know everyone works differently and when I’m facing a coding challenge the first thing I do is try my best to just get it to work. I don’t care how the code looks or anything. The only goal is to make it work. Here is my first working attempt at solving the problem of grabbing the CC’s:
/**
*
* Returns an ​array of email addresses
* ['john@example.com', 'jane@example.org'];
*
*/
public function getCC()
{
if ($ccs = $this->getHeaderByKey('cc')) {
$adrListArray = imap_rfc822_parse_adrlist($ccs, null);
$ccs = [];
foreach ($adrListArray as $item) {
$ccs[] = $item->mailbox.'@'.$item->host;
}
return $ccs;
}
return false;
}
This code is using PHP’s imap_* functions to first see if the CC header is set in the .eml file, then the imap_rfc822_parse_adrlist function to parse an email address string. This is useful to normalize​ the output of an email address list. The getHeaderByKey either returns the string of the header or false.
After I got it working the temporary variable was really annoying me, so I next refactored to the following:
public function getCC()
{
if ($ccs = $this->getHeaderByKey('cc')) {
$adrListArray = imap_rfc822_parse_adrlist($ccs, null);
return array_map(function($item){
return $item->mailbox.'@'.$item->host;
}, $adrListArray);
}
return false;
}
That started feeling better, but I really don’t like that all that code nested inside the if.
public function getCC()
{
if (! $ccs = $this->getHeaderByKey('cc')) {
return false;
}
$adrListArray = imap_rfc822_parse_adrlist($ccs, null);
return array_map(function($item){
return $item->mailbox.'@'.$item->host;
}, $adrListArray);
}
Now it feels cleaner to me and could be further simplified by removing the $adrListArray temporary variable:
public function getCC()
{
if (! $ccs = $this->getHeaderByKey('cc')) {
return false;
}
return array_map(function($item){
return $item->mailbox.'@'.$item->host;
}, imap_rfc822_parse_adrlist($ccs, null));
}
Personally, I think it’s cleaner than what I had the start, but even now I’m not 100% happy with it. I don’t like the $ccs variable assignment in the if statement, I think the method name should be changed to better represent this returning an array of email addresses. It’s not a single CC.
I think it’s a fun exercise writing out why you made the improvements that you did and thinking through how to make your code readable. I know this is subjective, but I find the last example easier to grasp vs. ​the first. What do you think? Do you see any other improvements that could be made?
Eric L. Barnes 
I think you should do something like this:
LikeLike
Hey Eric,
Your entry was featured in the latest PHP Weekly, that’s how I got here.
[rant on] I, personally, prefer iterating arrays in a foreach loop to using the array_* functions. This must be a generation-thing, I was growing up with loops, the functional approach came too late to my life, so I find loops clearer and more readable. 🙂 [rant off]
That being said, I’d take the foreach part and put it into a separate protected/private service method with an addressList array parameter to extract email addresses from an addressList. Could be reused later in a getBCC function, for example. The remaining part of the getCC() method would be a return statement calling this address-extracting service method. I’d also consider returning an empty array instead of false, in case there are no CS addresses.
LikeLike
I would rewrite it to something like this:
LikeLike
Is it possible to edit comments and to format code?
LikeLike
I just did it manually. Thanks!
LikeLike