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

Remove redundant code from abstract class. #194

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jcourtney1
Copy link
Contributor

AbstractApnsService had a lot of redundant code in it. I consolidated the core functions in a couple of methods and then had the other API methods with variant parameterizations call these rather than duplicating the implementations. The results seem much cleaner and easier to follow. I also expanded the API slightly to make specifying the expiry as a Date available as a variant of the methods with byte[] parameters and specifying the expiry as a Unix time available as a variant of the methods with String parameters.

…ing expiry as a Date should only be relevant to tokens and payloads specified as Strings or that expiry specified as a Unix timestamp (int) should only be relevant to tokens and payloads specified as byte[] so both combinations are now supported.
@matzew
Copy link
Collaborator

matzew commented Sep 16, 2014

great effort, @jcourtney1 ! I will review it asap.

BTW. do you mind adding some unit tests for the new methods you put on the ApnsService?

@jcourtney1
Copy link
Contributor Author

@matzew
I'd be happy to add unit tests for the new methods except I admit I'm not sure how. Currently it appears that only one send method on ApnsService of the 8 which existed prior to my change is being called in the unit tests. This method is ApnsService.send(String, String). These calls will also test AbstractApnsService.send(String, String, int) since this is called by AbstractApnsService.send(String, String). Unfortunately no pattern currently exists in the tests for testing the other methods that I can see and I'm not familiar with the Mokito way of things so my imagination on how to extend coverage to these is limited. If you've got some thoughts I'm happy to chip in and help make it happen but I'll need a bit of a jump start on the approach:)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants