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?
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