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?

5 thoughts on “ Simple PHP Refactoring ”

  2. 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.

    Like

  3. I would rewrite it to something like this:

    
public function getCC(): array
{
    if ($this->ccIsNotSet()) {
        return [];
    }

    return $this->covertAddressesStringToArray($this->getCcHeaderValue());
}

protected function ccIsNotSet(): bool
{
    if (!$this->getCcHeaderValue()) {
        return true;
    }

    return false;
}

protected function covertAddressesStringToArray(string $addresses): array
{
    return array_map(
        function ($item) {
            return $item->mailbox.'@'.$item->host;
        },
        imap_rfc822_parse_adrlist($addresses, null)
    );
}

protected function getCcHeaderValue(): ?string
{
    return $this->getHeaderByKey('cc');
}

    Like

Comments are closed.