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