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

Add more semantic constructors #67

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

haampie
Copy link

@haampie haampie commented May 10, 2016

This PR adds two methods which allow a more intuitive way to construct a parser, e.g.:

$parser = Parser::fromUserAgent($request->header('User-Agent'));
$parser = Parser::fromHeaders($request->headers());

As a side note: two redundant instanceof tests are removed. An instance either fails to construct or is already an instance of that class.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling e60a1e7 on haampie:feature-static-constructors into fcec4e9 on WhichBrowser:master.

@ThaDafinser
Copy link
Contributor

Different ways to achieve the same result are generally not good.
Also static calls should not be used to return instances of something

Also there is the analyse() method with the same signature.
https://github.com/haampie/Parser/blob/feature-static-constructors/src/Parser.php#L55

@haampie
Copy link
Author

haampie commented May 10, 2016

Different ways to achieve the same result are generally not good.
Also static calls should not be used to return instances of something

Named constructors are actually quite common (e.g. here or here). What is wrong with the concept in your opinion?

Also there is the analyse() method with the same signature.

Maybe this is a matter of taste, but I would rather not have such a method; it makes the state of the object mutable / not fully encapsulated. The method could be private for instance.

But this method is there probably because the constructor is expensive as well? In that case, must the parser already be constructed with a user agent string? Is the user agent string actually part of the state of a parser?

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.

3 participants