+1 I think that making the application id and master secret part of the Sender makes sense.

Looking at the code example you posted, I was a little surprised to see that the SenderClient.Builder does not build a SenderClient but instead a JavaSender. Inspecting the code I can see that SenderClient implements JavaSender of course, but having to think about this just truck me as there might be room for improvement.

Would it be possible to move the methods declared in SenderClient that are missing from JavaSender into that interface?

For example, it looks like the proxy methods and the truststore methods only exist in SenderClient.

Was there a reason for leaving them out of the JavaSender interface?

If we moved the methods up into the interface we could then make the naming consistent, like JavaSender and DefaultJavaSender. I'm lousy at coming up with class/interface names so this might be horrible but would PushSender be a better name for the interface?

This might be a personal preference but I much prefer using a static method instead when creating a new Builder instance. There is one such static method in SenderClient but the Builder's constructor is still public making the usage above valid. What I'd prefer is that the Builder's constructor be private and provide static methods to start the build process:

PushSender client = DefaultPushSender.withRootServerURL("http://aerogear.example.com/ag-push")

.proxy("proxy", 8080)

.proxyType(Proxy.Type.HTTP)

.build();

PushSender client = DefaultPushSender.withConfig("/path/on/classpath/push-config.json").build();

PushSender client = DefaultPushSender.withConfig(inputstream).build();

Again, these might be personal preferences and others might disagree but I thought I'd post them just the same incase that offer different view.

/Dan

On 8 November 2014 13:59, Sebastien Blanc <scm.blanc@gmail.com> wrote:

Hi,
Currently in the Java Sender Library the pushApplicationId and the masterSecret are part of the UnifiedPushMessage object. I would like to move these 2 fields to the Sender object and add it to its Builder API, to have something like :
JavaSender defaultJavaSender = new SenderClient.Builder("http://localhost:8080/ag-push")
                .pushApplicationId("c7fc6525-5506-4ca9-9cf1-55cc261ddb9c")
                .masterSecret("8b2f43a9-23c8-44fe-bee9-d6b0af9e316b")
                .build(); 
Why ?
Regarding design, these 2 fields are more part of the sender rather than the message itself
This is a first step to externalize the configuration, so we could have later, for instance, `JavaSender defaultJavaSender = new SenderClient.Config("path.to.config(or resource stream")
What does it change ?
It only concerns the Java Sender, we do not touch the Rest Sender API here.
A Sender instance is now bounded to a particular PushApp, so if we want to send a message to another PushApp we should :
Create a new instance of the Sender
Expose setters to update the config.
So , wdyt ?
_______________________________________________
aerogear-dev mailing list
aerogear-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/aerogear-dev