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

RealtimeChannel: duplicated publish method #127

Closed
ricardopereira opened this issue Jan 13, 2016 · 9 comments
Closed

RealtimeChannel: duplicated publish method #127

ricardopereira opened this issue Jan 13, 2016 · 9 comments
Labels
bug Something isn't working. It's clear that this does need to be fixed.

Comments

@ricardopereira
Copy link
Contributor

Code completion:

screen shot 2016-01-12 at 19 58 00

The RestChannel is the base class of the RealtimeChannel. RestChannel has 4 publish methods that we already discussed on #121.

Problem:
The RealtimeChannel does not override those methods and use their own. It should override them and avoid calling the super implementation.

IMO, we should fix #121 first.

@ricardopereira ricardopereira added bug Something isn't working. It's clear that this does need to be fixed. realtime labels Jan 13, 2016
@mattheworiordan
Copy link
Member

Agreed that #121 should be fixed first

@tcard
Copy link
Contributor

tcard commented Jan 14, 2016

I think the deeper problem is, why is RealtimeChannel inheriting from RestChannel? You can't use a RealtimeChannel as a RestChannel. If anything, both should inherit from a BaseChannel or implement a Channel protocol.

I strongly think that this should be fixed.

@mattheworiordan
Copy link
Member

I think the deeper problem is, why is RealtimeChannel inheriting from RestChannel

This is mostly because they share functionality such as history. However, as you say, that could be part of the BaseChannel implementation instead.

@tcard
Copy link
Contributor

tcard commented Jan 14, 2016

To reuse code, it is a very well-established "principle" to use composition over inheritance. Anyway, to not get sucked into the rabbit hole, I would be happy with inheriting from a BaseChannel.

@mattheworiordan
Copy link
Member

To reuse code, it is a very well-established "principle" to use composition over inheritance.

Sure, we do that in our Ably libraries. If that's easy enough to do in Objective-C, sure, although I suspect we're largely using inheritance elsewhere

@ricardopereira
Copy link
Contributor Author

Yes, it is largely using inheritance, e.g.: ARTBaseMessage.
I liked the use composition to fix this more than the ARTBaseChannel but I can do it for now.

@tcard So you mean something like ARTChannelHistory.history(...)?

@tcard
Copy link
Contributor

tcard commented Jan 14, 2016

@ricardopereira I guess. Or even RealtimeChannel having a RestChannel property, because AFAIK RealtimeChannel just calls the REST API for history, right?

@ricardopereira
Copy link
Contributor Author

Yes, you right.

@tcard
Copy link
Contributor

tcard commented Feb 19, 2016

Fixed by 97f0b16.

@tcard tcard closed this as completed Feb 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working. It's clear that this does need to be fixed.
Development

No branches or pull requests

3 participants