Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplified regex match storing code #398

Merged
merged 1 commit into from Aug 25, 2020

Conversation

Bilge
Copy link
Contributor

@Bilge Bilge commented Aug 22, 2020

Rather than first writing matches to a temporary variable, we write them directly to the class member variable. This eliminates the temporary $matches variable and also the temporary $result variable.

Note that we also eliminated the if statement which is intentional because it is this author's belief that it is incorrect for two reasons:

  1. It should not be conditional whether $matches are saved; if the method is called then $matches should always be updated rather than persist the result from a previous call.
  2. As an aside, and although not addressed by this PR, it is this author's strong belief that storing transient data, such as regex matches, between method calls is an anti-pattern because it is not concurrency-safe. Such runtime information should be returned immediately by the method. However, this refactoring is left for future work and affects the entire class design, not just this method.

@JayBizzle
Copy link
Owner

I will add the following test to make sure $matches doesn't persist.

    /** @test */
    public function matches_does_not_persit_across_multiple_calls()
    {
        $this->CrawlerDetect->isCrawler('Mozilla/5.0 (iPhone; CPU iPhone OS 7_1 like Mac OS X) AppleWebKit (KHTML, like Gecko) Mobile (compatible; Yahoo Ad monitoring; https://help.yahoo.com/kb/yahoo-ad-monitoring-SLN24857.html)');
        $matches = $this->CrawlerDetect->getMatches();
        $this->assertEquals($this->CrawlerDetect->getMatches(), 'monitoring', $matches);

        $this->CrawlerDetect->isCrawler('This should not match');
        $matches = $this->CrawlerDetect->getMatches();
        $this->assertNull($this->CrawlerDetect->getMatches());
    }

@JayBizzle JayBizzle merged commit f107a7b into JayBizzle:master Aug 25, 2020
@Bilge Bilge deleted the simplify-matches branch August 25, 2020 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants