Simple PHP Refactoring

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:

[code lang=text]
/**
*
* 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;
}
[/code]

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:

[code lang=text]
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;
}
[/code]

That started feeling better, but I really don’t like that all that code nested inside the if.

[code lang=text]
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);
}
[/code]

Now it feels cleaner to me and could be further simplified by removing the $adrListArray temporary variable:

[code lang=text]
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));
}
[/code]

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?