centrifugal / centrifuge-swift

Swift client SDK for bidirectional real-time communication with Centrifugo and Centrifuge-based server over WebSocket
MIT License
47 stars 41 forks source link

Code reorganization #3

Closed vadym-kozak closed 5 years ago

vadym-kozak commented 5 years ago

I made small code reorganization:

vadym-kozak commented 5 years ago

Also better demo added

FZambia commented 5 years ago

@vadimkozak hi, thank you, great to see interest to this library!

Just start looking at changes, you are using this code in demo:

guard let status = self.client?.status else {return}
if status == .new || status == .disconnected {

At this place you rely on non public status property. I'd like to keep this property private for now, so maybe it's better to rely on connect and disconnect events at moment to manage app state.

Also could you tell me what //MARK - mean?

vadym-kozak commented 5 years ago

hi @FZambia, Agree with you about access to ‘status’ property. But it was internal before. //MARK - is used for bettercode navigation and separate different parts of code to different sections.

FZambia commented 5 years ago

Agree with you about access to ‘status’ property. But it was internal before.

I mean that you should not use it in demo because library users won't have access to it. Maybe I am missing sth.

vadym-kozak commented 5 years ago

@FZambia thanks, did it here 545d474

FZambia commented 5 years ago

@vadimkozak thanks, one more thing is that getSubscriptionToken should remain private - it's not used by library users.

FZambia commented 5 years ago

@vadimkozak thanks! But refreshWithToken also internal func :) Token refresh mechanism fully based on delegate method of CentrifugeClientDelegate - onRefresh

FZambia commented 5 years ago

rpc and send methods should remain public, history method should remain private. the same for unsubscribe, presence and presenceStats, resubscribe

Also wrong comment near disconnect method.

vadym-kozak commented 5 years ago

@FZambia

Also wrong comment near disconnect method.

yes, thanks

But refreshWithToken also internal func :)

How user can reconnect with new token?

rpc and send methods should remain public

Which purpose of send function?

history method should remain private. the same for unsubscribe, presence and presenceStats, resubscribe

Why all this function should be private?

And wouldn't be better to describe all public function by using protocol?

FZambia commented 5 years ago

Which purpose of send function?

It allows to send async message to server, this is possible when backend based on Centrifuge library.

Why all this function should be private?

Because those methods relate to Subscription which has all these methods public. All those inside Client are just internal helpers.

And wouldn't be better to describe all public function by using protocol?

Is this a convenient way for Swift libs to declare public API?

vadym-kozak commented 5 years ago

Understood, thanks for clarification!

Is this a convenient way for Swift libs to declare public API?

There are no some docs or something about that. Someone use it to describe public functions like in Java, someone not.

FZambia commented 5 years ago

Someone use it to describe public functions like in Java, someone not.

Will try to think about this but for now I suppose we can skip this step.

Thanks a lot for your improvements, will merge changes and then I need to resolve some conflicts with my other branch with several important fixes (#4 ).

vadym-kozak commented 5 years ago

Thank you for your project, its simply great!