Complexity reduction

[EmanueleMinotto]

To solve the Facebook\Facebook constructor complexity problem (defined on Scrutinizer), I've splitted it into different factories:

• Facebook\PseudoRandomString\PseudoRandomStringGeneratorFactory
• Facebook\HttpClients\HttpClientsFactory
• Facebook\PersistentData\PersistentDataFactory

This should also allow you to move different detection logic into these factories and keeping some other classes' focus on their behaviour and responsabilities.

Another small change I did is the removal of an \InvalidArgumentException exception replicating a setter logic (afaik).

Of course I'll add related tests and documentation if the RFC is accepted.

[yguedidi]

👍 for reducing the complexity, 👎 on how it's done here.

I don't think we want those factories beind part of the SDK API.
I think you should move factrorie to private function of the Facebook master class.

[EmanueleMinotto]

Imo depends on "how the final developers should use the library", because if the Facebook\Facebook must be the only starting point for them, than ok, but if the final developer can start from Facebook\FacebookBatchRequest (if they already know that they want just use the batch requests system) than the factories logic can be reused.

@yguedidi what do you think about this?

Of course no problem if I should change the system to private methods, the goal is the same. :)

[yguedidi]

I think that any guessing should go in the master class, the easy to use starting point for developers, and any internal thing (or call it "advanced usage") should be type hinted.

In the example of Facebook\FacebookBatchRequest, the advanced user should know he/she needs a Facebook\FacebookClient, that needs a Facebook\HttpClients\FacebookHttpClientInterface. Then he/she will find one or will implement one. :)

[stof]

this condition can be simplified as 'stream' === $handler, with the same behavior, but a smaller cyclomatic complexity when computing it: you will only have 2 cases for this condition (the stream string vs anything else) instead of having 3 cases (the stream string vs any other string vs any other type)).

[stof]

@yguedidi having separate factories makes sense at least for the guessing of the best implementation, as it makes the logic usable even when you use directly the lower-level API (instead of having to duplicate the logic).
And the current implementation makes it possible as HttpClientsFactory::create(null).
Btw, it may make sense to allow omitting the argument for such case if you decide that it is a meaningful use case. I would also change the name create, because a static create method often creates an instance of the class itself, while it creates an HttpClient here, not an HttpClientsFactory.

[SammyK]

I'm down with the factories. There was another guy I was showing the source code to at a recent PHP conference who suggested using factories as well.

I would also change the name create, because a static create method often creates an instance of the class itself

@stof What convention are you referring to? On "php the right way" the example uses a create() method in a factory.

[EmanueleMinotto]

@stof first of all: thank you for all those corrections, applied and now it should be ok.
I've also understood what you mean about the create method, but I have no idea for other names (considering the repeated pattern of "automatic extraction of available tools" a common interface isn't so far, imo), do you have a suggestion for an alternative name?

PS: @gfosco I'm waiting your 👍 or 👎 before going ahead with tests and documentation.

[gfosco]

You have my 👍 😃

[stof]

@EmanueleMinotto I would use createHttpClient here (and similar for other)

[stof]

a common interface isn't so far, imo

It is far. the only interface you could extract in your current implementation would be create(mixed $arg): mixed, which does not make much sense.

[EmanueleMinotto]

@stof you're right, now it's senseless an interface for them, thanks again.

@gfosco updated with tests and documentation, considering the internal usage I didn't touch docs/ files, just added a line to the changelog, is it ok? I'm not sure about how to proceed for the documentation.

[SammyK]

👍 No need to worry about docs. All changes are internal and won't the public API unless we wanted to document the factories. But I think it might confuse newbies so if we decide to document it, it should probably be added to the reference only and not part of the "getting started" or anything.

Thanks for helping clean up the codebase @EmanueleMinotto!

[EmanueleMinotto]

my pleasure @SammyK :)

[gfosco]

Cool! 👍