Extensible DeviceFactory about javapns HOT 10 CLOSED

Making the DeviceFactory & PushNotifcationManager constructor public will break 
the singleton pattern.

As per DeviceFactory notes:
 * It implements the singleton pattern so only one class manages the devices,
 * avoiding problems with duplicate or lost devices

I can understand the desire to add JPA support, but it seems like making the 
PushNotifcationManager an abstract class would be more in line with the end 
goal. This will have to be investigated.

Thanks for reviewing this issue!  I'm not sure I understand though why the 
singleton pattern is so important to preserve here, as this is an anti-pattern 
in regards to dependency injection and makes unit testing far more difficult 
down the road - and is directly preventing users of the library from extending 
the DeviceFactory to provide their own implementations of Device.

Since PushNotificationManager always invokes DeviceFactory.getInstance(), I 
would strongly suggest removing the strict singleton requirement and allow 
users of the library to subclass DeviceFactory and statically set the actual 
instance that is used with DeviceFactory.setInstance(customFactory).  
PushNotificationManager would still always rely on a single instance of 
DeviceFactory (because they call the static getInstance method), alleviating 
the concerns about duplicate or lost devices.

The other way to go would be, as you suggested, to allow the 
PushNotificationManager class to be subclassed.  Again, I'm not sure I 
understand why the singleton pattern has any usefulness for this class (it 
seems to be causing more issues than it is solving, particularly for IoC and 
testing) . The PushNotificationManager would not need to be made abstract 
either, as a subclass could simply override your default implementations of 
addDevice, getDevice and removeDevice methods without it being abstract.  Plus 
if you don't make it abstract but make it subclassable, the library will be 
easier to use out-of-the-box for first-time users, while providing much more 
flexibility for users that cannot rely on a simple memory-mapped and volatile 
list of devices.

Thank you very much for all the efforts you put into this library!

> I'm not sure I understand though why the singleton pattern is so important
I don't really understand either, but I didn't write the code either. But there 
is a comment that says:
 * It implements the singleton pattern so only one class manages the devices,
 * avoiding problems with duplicate or lost devices

so, maybe this was made when the PushNotificationManager was not a singleton... 
dunno. 

A year or so ago, the owners of this project were gracious enough to add me to 
the project. I have been trying to fix bugs and add documentation to help 
others, but I have limited experience with Java. I also have limited time atm.

I talked it over with a co-worker and we think you have some good ideas. We 
agree with removing the singleton requirement on the DeviceFactory and making 
it into an abstract class so that it can be overridden.

If you would like to submit code or even get added to the project, then these 
changes could be expedited. 
I would very much welcome the assistance!

Hi!  I'd be happy to help with the evolution of this code, as it will be very 
useful within the larger project I work with. So yes, you are welcome to add me 
to the project.  Thank you!

We have begun working on enhancing the library to make it usable in a 
dependency-injected and JPA-persisted context.  The required changes involved 
many classes, whose documentation and test cases would need to be updated 
accordingly.  Thus to avoid modifying the trunk with a lot of sudden 
behavior-impacting changes, we've branched the project and committed all 
modifications to that branch instead.  

We believe that the ideal solution to support custom DeviceFactories was to go 
with interfaces.  Going with interfaces instead of abstract classes makes the 
library even easier to use, because it doesn't require users to extend a class 
(which might not be possible in many situations).

Further more, the current project structure probably made sense for the 
originally intended use of the library;  but in a dependency-injected and 
JPA-persisted project, a significant refactoring effort helped make clear 
separations of roles and responsibilities of the various parts of the library.  
Again, this has been committed to the new branch only, for review purposes, so 
not to cause panic on the main source tree :)

How is the enhancement going?

Hi, other parts of our project required our attention for the past few weeks, 
but we'll be getting back to Javapns integration very shortly.  I'll keep you 
informed of our progress :)  Thank you!

Have you seen:
https://github.com/notnoop/java-apns/wiki

Looks like they have some of the features you want.

[deleted comment]

Made a major commit to the 1.7 branch today (see r160 and r161).  As it is now, 
this release pushes notifications perfectly for us, and can be hooked up 
successfully with JPA (although this is still optional).  It also improves 
error reporting, so it is easier to understand what is going wrong in some 
situations (such as bad keystore password, invalid certificate chain, etc.).  
This release also fixes a critical bug in the PushNotificationManager class 
which also affects the trunk (pushing empty payloads instead of the supplied 
ones).  The Feedback Service is also working and deleting devices in the 
database as needed.  All recent changes to the trunk have also been imported 
into this branch.