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

Implementation should be explicit #108

Closed
astrokin opened this issue Nov 17, 2016 · 5 comments
Closed

Implementation should be explicit #108

astrokin opened this issue Nov 17, 2016 · 5 comments
Labels
status: work in progress integration is in progress / being tested type: enhancement

Comments

@astrokin
Copy link

// MARK: Singleton
    private static let sharedInstance = SwiftyStoreKit()

There is no any reason to do this. Please think about about it in unit-tests perspective and Open/closed principle. I would recommend to let users of your software have more control and modify it's state.

@bizz84
Copy link
Owner

bizz84 commented Jan 21, 2017

Slowly getting there: #131

I think having an init() method with default parameters for dependencies would help with unit tests, however I would still leave the public facing API as is (with any singleton / state private inside the library). What do you think?

@astrokin
Copy link
Author

@bizz84 you did a really great job. from the development perspective it would be greate to refactor a little. I did already in my fork because without these changes i got a crash in release configuration with RxSwift wrapper around your library. We can discuss even more deep refactoring to avoid singleton instance.

@bizz84
Copy link
Owner

bizz84 commented Jan 21, 2017

I think it's nicer to write:

SwiftyStoreKit.methodName(params)

rather than:

SwiftyStoreKit.shared.methodName(params)

In the SwiftyStoreKit class, I am planning to have each public class method call a corresponding instance method via the shared singleton.

By exposing a public init() method as well as other public methods, it would be possible to create an instance of SwiftyStoreKit and bypass the singleton entirely.

The question is: is this desirable from an API usage standpoint? This would mean that client code would have to create and retain an instance of SwiftyStoreKit somewhere. In my use cases I always call SwiftyStoreKit from the app delegate as well as from specific view controllers where the purchase flows are hooked up, so having class methods and an internal singleton is quite nice.

Feedback?

@bizz84 bizz84 added the status: work in progress integration is in progress / being tested label May 18, 2017
@bizz84
Copy link
Owner

bizz84 commented May 18, 2017

@astrokin I'm slowly making all components inside SwiftyStoreKit testable and adding unit tests.

Eventually I would like SwiftyStoreKit to be just a facade that forwards all the method calls to the right components via composition.

@bizz84
Copy link
Owner

bizz84 commented Dec 29, 2017

Closing this issue due to inactivity. Thanks for the feedback @astrokin, more suggestions are welcome.

@bizz84 bizz84 closed this as completed Dec 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: work in progress integration is in progress / being tested type: enhancement
Projects
None yet
Development

No branches or pull requests

2 participants