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

Add Network component #58

Merged
merged 8 commits into from Mar 10, 2018

Conversation

takuyakanbr
Copy link

@takuyakanbr takuyakanbr commented Mar 10, 2018

  • Add GoogleBooksApi class that allows searching books and obtaining book details using Google Books API
  • Add request and result events for searching books and obtaining book details using Google Books API
  • Add new dependency async-http-client

* Add class GoogleBooksApi that allows searching books and obtaining getting book details using Google Books API
* Add request and result events for searching books using Google Books API
* Add new dependency async-http-client
* Add methods and deserializers to convert JSON response from Google Books API into a Book / BookShelf
* Add wrapper classes for AsyncHttpClient and Response from async-http-client to allow easier unit testing
@takuyakanbr takuyakanbr added the status.completed Ready to be reviewed label Mar 10, 2018
Copy link
Collaborator

@qiu-siqi qiu-siqi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure its a good idea to explicitly name the events with GoogleApi...


/**
* Returns the URL encoded form the string {@code s}.
* @throws NullPointerException if {@code s} is null.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to mention?

return URLEncoder.encode(s, "UTF-8");
} catch (UnsupportedEncodingException e) {
return "";
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should mention in method description that it returns empty string if invalid

* @return a CompletableFuture that resolves to a ReadOnlyBookShelf.
*/
public CompletableFuture<ReadOnlyBookShelf> searchBooks(String parameters) {
String requestUrl = URL_SEARCH_BOOKS.replace("%s", StringUtil.urlEncode(parameters));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use String.format(...)

* @return a CompletableFuture that resolves to a Book.
*/
public CompletableFuture<Book> getBookDetails(String bookId) {
String requestUrl = URL_BOOK_DETAILS.replace("%s", StringUtil.urlEncode(bookId));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use String.format(...)

return httpClient
.makeGetRequest(url)
.thenApply(response -> {
assert response.getContentType().startsWith(CONTENT_TYPE_JSON);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

supposed to crash?

* Remove 'Google' from the name of API request and result events
* Use String.format to create request urls from url constants
@@ -59,7 +59,10 @@ public GoogleBooksApi(HttpClient httpClient) {
return httpClient
.makeGetRequest(url)
.thenApply(response -> {
assert response.getContentType().startsWith(CONTENT_TYPE_JSON);
if (response.getContentType().startsWith(CONTENT_TYPE_JSON)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not?

@qiu-siqi qiu-siqi merged commit f3a8030 into CS2103JAN2018-F14-B4:morph Mar 10, 2018
@qiu-siqi qiu-siqi added status.accepted Accepted and removed status.completed Ready to be reviewed labels Mar 10, 2018
@takuyakanbr takuyakanbr deleted the add-google-books-api branch March 10, 2018 18:53
@takuyakanbr takuyakanbr added the type.task Something that the developer can do label Mar 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status.accepted Accepted type.task Something that the developer can do
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants