search

jforrest / Chargify-PHP-Client

PHP client for Chargify
Please let me know if you have any questions.
MIT License
19 stars 16 forks source link

refactor connection usage #7

Open makasim opened 13 years ago

makasim commented 13 years ago

I've made some changes in ChargifyConnector useage.

Now all object should receive connector as a fitst constructor parameter.

This change makes the library more flexibal.

You can have as many connectors as you want (two connectors: test and prod for example). Also it makes the library easy to test.

I would very happy ifyou add this changes to your branch.

jforrest commented 13 years ago

Thank you!

I will have some time this weekend to integrate your changes. I will shoot you an email once I finish.

On Thu, Mar 24, 2011 at 10:22 AM, makasim < reply@reply.github.com>wrote:

I've made some changes in ChargifyConnector useage.

Now all object should receive connector as a fitst constructor parameter.

This change makes the library more flexibal.

You can have as many connectors as you want (two connectors: test and prod for example). Also it makes the library easy to test.

I would very happy ifyou add this changes to your branch.

Reply to this email directly or view it on GitHub: https://github.com/jforrest/Chargify-PHP-Client/pull/7

makasim commented 13 years ago

Sometime past and there is not any news from you. I would like to hear your feedback to my changes. Are you going to merge?

Thanks!

ziemkowski commented 13 years ago

I was just going to create an issue saying there was no way to set connection information without editing the source files, which makes updating more prone to issues. Your changes fix that issue since I can just have a custom class that returns a connector that any other file can then use without knowing the connection information and without editing any of the source files.

Read the diffs and they look good. +1 for pulling in these changes.

One question though, since this change isn't backwards compatible, why not take the opportunity to make the factory methods actually static? For example, you still have to create a subscription with a connection, and then replace the instance with one returned by getByID(), getByCustomerID(), or getAll(). Would definitely be cleaner to have ChargifySubscription::getByID($connector, $id); for instance.

Going even further, I could see swapping the parameter order so $connector is last and optional with a static ChargifyConnector::$default_connector used if null (which could be populated by say the last created connector or via a method that makes the connector default).

makasim commented 13 years ago

I don't like the idea of factory method. Because:

1) It makes dependency between connector and api classes less clear. 2) It is library and if it so it should be as much flexible as it possible, 3) The default connector can be handled by an application api layer which stands above the lib.

But I see the way to archive what you want. I think it can be a ChargifyConnectorManger (like it in Doctrine) which can be used by api classes.

Thanks for support.

ziemkowski commented 13 years ago

I shouldn't have used the term factory, I was using it loosely to mean they're creation methods. It just seems odd that they're instance methods creating new instances, instead of say a static method.

So I have to create an instance, then replace it with the method's new instance, and then when I call some other method, the connector creates yet another instance.