Skip to content

Implement better "java state of the art" API #126

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

Closed
aholbreich opened this issue Aug 3, 2016 · 12 comments
Closed

Implement better "java state of the art" API #126

aholbreich opened this issue Aug 3, 2016 · 12 comments
Labels
status: help wanted requesting help from the community type: community enhancement feature request not on Twilio's roadmap

Comments

@aholbreich
Copy link

aholbreich commented Aug 3, 2016

Instead of

    Email from = new Email("[email protected]");
    String subject = "Hello World from the SendGrid Java Library!";
    Email to = new Email("[email protected]");
    Content content = new Content("text/plain", "Hello, Email!");
    Mail mail = new Mail(from, subject, to, content);
...

SendGrid sg = new SendGrid(System.getenv("SENDGRID_API_KEY"));
    Request request = new Request();
    try {
      request.method = Method.POST;
      request.endpoint = "mail/send";
      request.body = mail.build();
      Response response = sg.api(request);
      System.out.println(response.statusCode);
      System.out.println(response.body);
      System.out.println(response.headers);
    } catch (IOException ex) {
      throw ex;
    }

ist should look like

    Email from = new Email("[email protected]");
    String subject = "Hello World from the SendGrid Java Library!";
    Email to = new Email("[email protected]");
    Content content = new Content("text/plain", "Hello, Email!");
    Mail mail = new Mail(from, subject, to, content);
...

SendGrid sg = new SendGrid(System.getenv("SENDGRID_API_KEY"));
//Hiding technical details of request (they always look the same)
    try {
      MailSendResponse response = sg.mailSend(mail.build()); //If needed having special Responses dealing with nuances of API endpoints  
      System.out.println(response.getStatusCode()); //Public get methods instead of public memebers.
      System.out.println(response.getBody());
      System.out.println(response.getHeaders());
    catch (IOException ex) {
      throw ex; 
    }

as example. Discussible in details.

@thinkingserious thinkingserious added type: community enhancement feature request not on Twilio's roadmap status: help wanted requesting help from the community labels Aug 4, 2016
@thinkingserious
Copy link
Contributor

Thanks @aholbreich,

I like all of your suggested changes. I'm looking forward to implementing during the refactor of the Mail Send helper.

@caryy
Copy link

caryy commented Sep 8, 2016

@thinkingserious my coworkers and I would also definitely appreciate this refactor.

One request I have is to please retain the "fluent" style of the 2.2.2 client, (where you can chain method calls together, by having mutator methods return the object that they mutate. See https://github.com/sendgrid/sendgrid-java/blob/v2.2.2/src/main/java/com/sendgrid/SendGrid.java#L209 as an example).

Honestly, the API of the 2.2.2 client was very good, and is, IMO, a worthy reference — if not an ideal — for how this client should behave. As it was written, it supported both the (still relatively pleasant) multi-line style illustrated in @aholbreich's example, as well as chaining calls together like so:

Sendgrid.Email email = new Email()
   .setFrom("[email protected]")
   .setTo("[email protected]")
   .setSubject("I thought you would like this email!")
   .setBody("I hope you are having a good day");
SendGrid.Response response = sendGridClient.send(email);

Ideally, I feel that a consumer of this client shouldn't need to know anything about the under-the-hood REST machinations (content type, method, endpoint, etc.) — it should be as simple as configuring a client object, passing that client object an "email" object, and receiving a Response object that represents the success or failure of the operation. Admittedly, my use cases so far have been relatively simple, so maybe your power-users are scoffing at such a suggestion — regardless, I'm of the opinion that the more abstraction of the actual REST nitty-gritty you can provide, the better.

Finally, I want to be clear that I'm not trying to trivialize the hard work I'm sure you've put into the 3.0 version, and I hope I don't come off as overly pedantic. I just thought it would be helpful to provide some feedback about what I liked about the prior release, as a user.

@thinkingserious
Copy link
Contributor

Hello @carry,

Thank you for the great feedback! We definitely agree with "a consumer of this client shouldn't need to know anything about the under-the-hood REST machinations" and that's the direction we are headed. The Mail Helper is the first step in that direction. We took a few steps back to gain support for the v3 API and we are now looking forward to iterating along with community feedback.

Thanks for the kind words and for your time and great feedback please email us at [email protected] with your T-shirt size and mailing address :)

Same for you @aholbreich if we have not already done so.

Also, I've added your vote to the issue to help get it higher in our backlog. Thanks!

@aholbreich
Copy link
Author

@caryy i like your improvements 👍 i think in same direction.

I don't know well other parts of the API beyond the sending endpoint. But there is a lot according to documentation. Therefore in my opinion this part
SendGrid.Response response = sendGridClient.send(email);
need to be done carefull.

First Response need to hanlde specific responses of all API parts. I gess there are some that may return some statistic for example.
Also send() is to specific. If think the API is devided logicaly in different parts (resourses) so it might be better idea to reflect that parts in the Java Client API a well.

SendGrid.Response response = sendGridClient.Mailer.send(email);
or as example (don't know it that exists) Other Use Case.:
SendGrid.ContentResponse response = sendGridClient.SendingEndpoint.Metric.getBounced(selectorl);

Here ContentResponse given but can be subclass of Response. Also Mailer and Metric entpoins could make sence.

Hope this makes some sense. :)

@thinkingserious
Copy link
Contributor

Hello @caryy, @aholbreich,

Since you are some of the few that have providing amazing feedback, I thought you might be interested in this: #140 :)

@RichardCSantana-zz
Copy link

So I made this pull request trying to answer this issue, could you look at it ?

#149

@thinkingserious
Copy link
Contributor

@richardcsantana,

I replied to your pull request, thanks!

You may also be interested in the Mail Helpers project: https://github.com/sendgrid/sendgrid-java/projects

@gilles
Copy link

gilles commented Dec 12, 2016

you can use https://github.com/danielnorberg/auto-matter or https://github.com/google/auto/tree/master/value for your POJOs

@thinkingserious
Copy link
Contributor

Thanks for the feedback @gilles!

@nrktkt
Copy link

nrktkt commented Jan 5, 2017

Just a 👍 for all of this, and a quick list of things I'd change (some duplicates to what's already here)

  • Abstract away all the HTTP and REST, sg.send(mail.build()) instead of request object. Have the response object be a pojo with some meaningful fields rather than http status and json body.
  • Public methods instead of public members
  • Name Mail something like MailBuilder if it really is a builder
  • Fluent/chaining builder methods
  • Meaningful exceptions
  • Remove test dependencies from final binary (looking at you mockito in the http library)
  • If we wanted to be really state of the art, we could offer async operations and use something like AsyncHttpClient instead of apache http.

EDIT: more

  • Mail.build() should not throw exceptions. Java objects (except for builder style objects) should never be in an invalid state (Mail violates this), and it should be impossible for Jackson to have a problem building an object in a valid state, so build() should not need to throw a checked exception.

@thinkingserious
Copy link
Contributor

Thanks for the feedback @kag0!

I'm very much looking forward to the next iteration of this library :)

@thinkingserious
Copy link
Contributor

Hello Everyone,

Please follow this issue to track progress and provide feedback. Thanks!

With Best Regards,

Elmer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: help wanted requesting help from the community type: community enhancement feature request not on Twilio's roadmap
Projects
None yet
Development

No branches or pull requests

6 participants