current community

your communities

more stack exchange communities

Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite
1

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.

share|improve this question

1 Answer 1

up vote 4 down vote accepted

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
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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