Fix most common parsing failures when @ is present in the docblock but not for an annotation

[Seldaek]

Replaces doctrine/common#195 - in the most common path this just adds one $input[$pos] !== ' ' && $input[$pos] !== '*' check which I think is worth fixing this WTF-inducing-bug, really not much compared to the whole annotation parsing, and it's cached anyway right?

[Seldaek]

Also this replaces #17

[Seldaek]

Also this means that the first annotation must be either at the beginning of the docblock, or preceded by or* to be valid. I believe this is acceptable?

[Seldaek]

And one last note, if you like instead of the while(true) I would be happy to use goto :p

[Ocramius]

Ping @guilhermeblanco

[guilhermeblanco]

Approach seems odd to me. This while(true) is extremely unreadable.

I'd do something like:

public function parse($input, $context = '')
{
    $tokenPosition = $this->findInitialTokenPosition($input);

    if ($tokenPosition === null) {
        return array();
    }

    // ...
}

private function findInitialTokenPosition($input)
{
    $position = 0;

    while (($position = strpos($input, '@', $position)) !== false) {
        if ($position > 0 && ! in_array($input[$position - 1], array(' ', '*'))) {
            $position += 2;

            continue;
        }

        return $position;
    }

    return null;
}

That would make code a little more readable. =)

[Seldaek]

Simplified the loop a bit and added comments.

[guilhermeblanco]

@Seldaek why not put this in a separate method?
You are increasing complexity of method parse and now creating continue/breakthat is hard to read code.

[Seldaek]

There, I swapped them so continue is gone, and if you argue that break is harder to read than return then I give up :)

[guilhermeblanco]

@Seldaek ok, I'll explain more detailed... =P

First, separating into an isolated method helps to make method parse do 1 thing only. This new method also have only one responsibility: find the initial token position for annotations. It helps testability, reduces complexity per method and consequently the "CRAP" phpUnit loves to display.

Second, is about code flow. We all know that early returns is one of the key strategies that helps Object Calisthenics rules to be met. But considering your code flow, your success case should always be the normal flow; error cases are branches of the normal flow creating alternate flows. I detailed this in the only blog post I've ever wrote in my life but focused on JS side: http://phpork.net/2012/08/jslint-and-continue-keyword/

This means that looking at your patch, the break condition is inside of an alternate flow and it is in reality your success case. This makes the code less readable. That's why I suggested originally a code that is still not perfect (the correct is a divergence), but it is quite there. If you really want, the correct solution would be something like (I haven't tested and I already imagine that non 0 initial token based could break):

private function findInitialTokenPosition($input)
{
    $position = strpos($input, '@');

    // @ not found
    if ($position === false) {
        return null;
    }

    do {
        if ($position > 0 && ! in_array($input[$position - 1], array(' ', '*'))) {
            $position++;
        }
    } while (($position = strpos($input, '@', $position)) !== false);

    return $position;
}

I hope this makes a bit more sense now. =)

[Seldaek]

You can't do a do .. while because this function will always return null or false. You need to break the loop at some point, either using a return or a break.. Extracting it in another function changes the keyword but that's about it.

Anyway as said, giving up, 39420e2 fixes it I guess. Don't see how this private method that will never be tested is more testable but whatever.

[guilhermeblanco]

@Seldaek I gave it a try and it does seem good enough for merging.
We can work out minor details later as this feature seems more important merged and working than discussing and delaying a minor CS cosmetic piece of code.
Thanks for you patience! =)

[Seldaek]

@guilhermeblanco thanks :)