1
\$\begingroup\$

I usually spent more time on refactoring than in developing itself. But this little piece of code is really hard to refactor. I'd like to keep it as short as possible, but it's quite undarstandable:

foreach($all_headers as $uid => $h) // Intera gli headers
{

    // Is the header valid?
    $isInbox   = $h->to == $mailbox->address;
    $hasPin    = ($pin = substr($h->subject, 0, 5)) !== false && is_numeric($pin);
    $hasTitle  = strlen($title = trim(substr($h->subject, 5))) > 0;
    $isAllowed = $hasPin && call_user_func($pincall, $mailbox->address, $h->from, $pin);

    $log->info("messaggio $uid: in:$isInbox pin:$hasPin alw:$isAllowed");

    // Skip this header if not vlaid
    if (!$isInbox || !$hasPin || !$isAllowed) continue;

    // Add some properties to current header
    $h->title = $hasTitle ? $title : null;
    $h->pin   = $pin;
    $valid_headers[$uid] = $h;

}

Since i'm filtering $all_headers array, one cool idea would be using array_filter with some kind of mechanism to add/remove conditions "on the fly".

Thank you people, as always.

\$\endgroup\$

1 Answer 1

4
\$\begingroup\$

This won't make it shorter, but you might consider changing the condition checking to only do the actual check if needed instead of doing all of them at once, making you loose the advantage of the short-circuiting || operator.

E.g.

if (!isInbox()) continue;
if (!hasPing()) continue;
...

Another possibility, that is even less short, is to create a filtering iterator, e.g.:

class ValidHeaderIterator extends FilterIterator 
{
    private $mailbox;
    private $pincall;   

    public function __construct($headers, $mailbox, $pincall)
    {
        $this->mailbox = $mailbox;
        $this->pincall = $pincall;
        parent::__construct(new ArrayIterator($headers));
    }

    public function accept()
    {
        $header = $this->getInnerIterator()->current();
        return isInbox($header) && hasPin($header) && isAllowed($header);
    }

    public function isInbox($header)
    {
        return $header->to == $this->mailbox->address;
    }

    public function hasPin($header)
    {
        $pin = $this->getPin($header);
        return $pin !== false && is_numeric($pin);
    }

    public function getPin($header)
    {
        return substr($header->subject, 0, 5));
    }

    public function isAllowed($header)
    {
        return call_user_func($this->pincall,
                              $this->mailbox->address,
                              $header->from,
                              $this->getPin());
    }
}

The calling code would then look like:

foreach(new ValidHeaderIterator($headers, $mailbox, $pincall) as $header) {
    // do stuff
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.