Month: March 2018

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?