feat: Removed volley and shifted api calls to retrofit #2896

codedsun · 2019-10-19T11:50:48Z

Fixed #2895
Changes: Removes volley and shifts to retrofit

Screenshots of the change:

codedsun · 2019-10-19T12:11:19Z

@iamareebjamal @yashk2000 Please merge this!

iamareebjamal · 2019-10-19T12:22:32Z

app/src/main/java/org/fossasia/phimpme/share/SharingActivity.java

-            Constants.IMGUR_IMAGE_UPLOAD_URL,
-            new Response.Listener<String>() {
+    ApiInterface retrofitClient =
+        RetrofitClient.getRetrofitClient(Urls.IMGUR_BASE_URL).create(ApiInterface.class);


This should be done once and stored in a static variable

We can't because we need to have retrofit client with multiple base Url @iamareebjamal

We can't because we need to have retrofit client with multiple base Url

Look at this, again
RetrofitClient.getRetrofitClient(Urls.IMGUR_BASE_URL)

Then look at this, again
RetrofitClient.getRetrofitClient(Urls.IMGUR_BASE_URL).create(ApiInterface.class)

And then read your comment

@iamareebjamal I have created client for Imgur but when I want to create for any other api I have to pass that api base url, for example pinterest.

We can't create a singleton class and store in static variable when we have multiple base url.

You can never use ApiInterface for any other base URL or even any other endpoint. If you can, I'll change my name to whatever you say

@iamareebjamal I know that we can create retrofit instance in a singleton class but this can be done when we have baseUrl only 1 through out the app. But as the pineterest account uses another base url I will have to create another singleton class for that base url as you suggested..

what i followed to not follow singleton approach and build the retrofit client with the base URL provided in the function itself.

ApiInterface is just an interface with different endpoints. You can invoke that function which corresponds to that baseUrl. It makes sense that with Imgur base URL i will not call uploadImagetoPinterest function and vice versa.

If i am wrong please guide me what shall I do.?

You want me to have multiple retrofit client in the app and mutiple interfaces for endpoints, I will go with this approach..

Retrofit instance creation uses reflection and is expensive and should be done only once per application or activity, not on each function call

@iamareebjamal I will create the retrofit instance in the activity itself and should I pass the URL from here or keep that where the retrofit instance is created?

Pass from here

app/src/main/java/org/fossasia/phimpme/utilities/ApiInterface.java

iamareebjamal · 2019-10-19T12:24:07Z

app/src/main/java/org/fossasia/phimpme/utilities/Urls.java

+/** Created by @codedsun on 19/Oct/2019 */
+public class Urls {
+
+  public static final String IMGUR_BASE_URL = "https://api.imgur.com";


Not useful as it is not reused

It's used while creating retrofit client

https://en.wikipedia.org/wiki/Reuse

This is an interesting read on this topic

@iamareebjamal We have one place for keeping all the API's URL.

i will keep all the other accounts' url api endpoints here, if you want me to remove the file and use that directly in activity or something i will do that.

That's a solid anti-pattern

codedsun · 2019-10-19T13:04:02Z

@iamareebjamal What i m getting is to have multiple servicesclass (API Endpoint) for every baseUrl and no Urls file as they are not re-used anywhere?

codedsun · 2019-10-19T15:15:16Z

@iamareebjamal You say to have a single retrofit instance for imgur, it's right but what shall I do when I have to hit with another base URL like for pinterest upload image, PR #2892. Then will I create another retrofit client. Then at runtime how will I change the base URL. Please define where I am wrong and what steps should i take to make the PR correct and merge ready.!

Thanks!

iamareebjamal · 2019-10-19T16:56:20Z

You have to create another retrofit instance for another base URL anyway, so I don't get your point at all. Why are you creating the retrofit instance and the service class both on each call of the function?

Imagine retrofit instance and service class instantiation takes 3 seconds to complete for the sake of argument. And rest of the method takes 1 sec. Your method will take 4 second each time and my method will take 4 second for first time and 1 second for every other time.

You say to have a single retrofit instance for imgur, it's right but what shall I do when I have to hit with another base URL like for pinterest upload image

Then you have to create another retrofit instance in any case! It has nothing to do with the fact that you are creating the service in method each time, instead of 1 time. You may use singleton or not, that is your choice. Both of those approaches will not save you from having to create another retrofit instance for pinterest upload

codedsun · 2019-10-19T18:34:05Z

@iamareebjamal Convinced, thankyou for such detail. Please check now

app/src/main/java/org/fossasia/phimpme/utilities/ImgurRetrofitClient.java

app/src/main/java/org/fossasia/phimpme/share/SharingActivity.java

codedsun · 2019-10-19T19:20:21Z

@iamareebjamal check now

iamareebjamal · 2019-10-19T20:34:32Z

app/src/main/java/org/fossasia/phimpme/utilities/ImgurApiInterface.java

+import retrofit2.http.POST;
+
+/** Created by @codedsun on 19/Oct/2019 */
+public interface ImgurApiInterface {


Suggested change

public interface ImgurApiInterface {

public interface ImgurApi {

iamareebjamal · 2019-10-19T20:34:38Z

app/src/main/java/org/fossasia/phimpme/utilities/ImgurApiInterface.java

+import retrofit2.http.Header;
+import retrofit2.http.POST;
+
+/** Created by @codedsun on 19/Oct/2019 */


app/src/main/java/org/fossasia/phimpme/utilities/RetrofitClient.java

codedsun · 2019-10-20T10:00:23Z

@iamareebjamal Check!

iamareebjamal · 2019-10-20T11:56:59Z

app/src/main/java/org/fossasia/phimpme/utilities/Constants.java

@@ -8,7 +8,6 @@
  public static final int REQUEST_SHARE_RESULT = 50;
  public static final String SHARE_RESULT = "share_result";

-  public static final String IMGUR_IMAGE_UPLOAD_URL = "https://api.imgur.com/3/image/";
  public static String IMGUR_HEADER_CLIENt = "Client-ID";
  public static String IMGUR_HEADER_USER = "Bearer";


If these are used in one place only, move them to that place as well

iamareebjamal · 2019-10-20T11:57:30Z

app/src/main/java/org/fossasia/phimpme/utilities/RetrofitClient.java

+import retrofit2.Retrofit;
+import retrofit2.converter.gson.GsonConverterFactory;
+
+/** Created by @codedsun on 19/Oct/2019 */


These are automatically generated. Ok will remove

iamareebjamal · 2019-10-20T11:57:47Z

app/src/main/java/org/fossasia/phimpme/utilities/RetrofitClient.java

+    HttpLoggingInterceptor interceptor = new HttpLoggingInterceptor();
+    interceptor.level(HttpLoggingInterceptor.Level.BODY);
+    httpClient.interceptors().add(interceptor);
+    return httpClient;


This is same for every instance, so it should be cached

iamareebjamal · 2019-10-20T11:58:02Z

app/src/main/java/org/fossasia/phimpme/utilities/RetrofitClient.java

+  private static OkHttpClient.Builder httpClient() {
+    OkHttpClient.Builder httpClient = new OkHttpClient.Builder();
+    HttpLoggingInterceptor interceptor = new HttpLoggingInterceptor();
+    interceptor.level(HttpLoggingInterceptor.Level.BODY);


Level BODY is too verbose

@iamareebjamal Just used for development purpose, will add check of debug and production here from buildconfig

BODY is too verbose even for debug. Stetho should be used instead

iamareebjamal · 2019-10-20T13:10:23Z

app/src/main/java/org/fossasia/phimpme/utilities/ImgurApi.java

+
+  @POST("/image")
+  Call<JsonElement> uploadImageToImgur(
+      @Header("Authorization") String authorization, @Body ArrayMap<String, String> body);


Change body to a class

why not arraymap?

Because it's java, not python or JS

iamareebjamal · 2019-10-20T13:10:33Z

app/src/main/java/org/fossasia/phimpme/utilities/ImgurApi.java

+public interface ImgurApi {
+
+  @POST("/image")
+  Call<JsonElement> uploadImageToImgur(


JsonElement to as well

? Response to a class? why?

Do we should have a class for every API response? why should we follow this approach?

See the extension of the file

i will change but JsonElement is also a class present under package com.google.gson;

Obviously, it is. You can even use ArrayList without type parameters to make it like Python and JS. You can do anything in Java that you can do in Python or JS, but we want type safety

codedsun · 2019-10-20T15:26:37Z

@iamareebjamal Review !

iamareebjamal · 2019-10-20T16:14:45Z

app/src/main/java/org/fossasia/phimpme/share/SharingActivity.java

  public static final String EXTRA_OUTPUT = "extra_output";
+  public static String IMGUR_HEADER_CLIENT = "Client-ID";
+  public String IMGUR_HEADER_USER = "Bearer";


Group IMGUR fields together

iamareebjamal · 2019-10-20T16:15:39Z

app/src/main/java/org/fossasia/phimpme/share/imgur/ImgurPicUploadResp.java

+    this.data = data;
+  }
+
+  public class Data {


Suggested change

public class Data {

public static class Data {

iamareebjamal · 2019-10-20T16:46:46Z

app/src/main/java/org/fossasia/phimpme/share/SharingActivity.java

@@ -155,6 +150,9 @@
        OnRemoteOperationListener,
        RecyclerItemClickListner.OnItemClickListener {

+  private static final String IMGUR_BASE_URL = "https://api.imgur.com/3/";
+  public static String IMGUR_HEADER_CLIENT = "Client-ID";
+  public String IMGUR_HEADER_USER = "Bearer";


Why are these public and not static final?

iamareebjamal · 2019-10-20T16:47:12Z

app/src/main/java/org/fossasia/phimpme/share/SharingActivity.java

@@ -155,6 +150,9 @@
        OnRemoteOperationListener,
        RecyclerItemClickListner.OnItemClickListener {

+  private static final String IMGUR_BASE_URL = "https://api.imgur.com/3/";
+  public static String IMGUR_HEADER_CLIENT = "Client-ID";
+  public String IMGUR_HEADER_USER = "Bearer";


Suggested change

public String IMGUR_HEADER_USER = "Bearer";

private static final String IMGUR_HEADER_USER = "Bearer";

iamareebjamal · 2019-10-20T16:47:25Z

app/src/main/java/org/fossasia/phimpme/share/SharingActivity.java

@@ -155,6 +150,9 @@
        OnRemoteOperationListener,
        RecyclerItemClickListner.OnItemClickListener {

+  private static final String IMGUR_BASE_URL = "https://api.imgur.com/3/";
+  public static String IMGUR_HEADER_CLIENT = "Client-ID";


Suggested change

public static String IMGUR_HEADER_CLIENT = "Client-ID";

private static final String IMGUR_HEADER_CLIENT = "Client-ID";

codedsun · 2019-10-20T17:17:44Z

@iamareebjamal are we good to go now?

codedsun · 2019-10-20T17:18:17Z

@iamareebjamal Thank you for approval and efforts!

auto-label bot added the Feature label Oct 19, 2019

iamareebjamal reviewed Oct 19, 2019

View reviewed changes

app/src/main/java/org/fossasia/phimpme/utilities/ApiInterface.java Outdated Show resolved Hide resolved

iamareebjamal reviewed Oct 19, 2019

View reviewed changes

codedsun force-pushed the Retrofit branch from 96bd532 to 0c8aaad Compare October 19, 2019 12:35

codedsun force-pushed the Retrofit branch from 0c8aaad to 5520a68 Compare October 19, 2019 18:32

iamareebjamal reviewed Oct 19, 2019

View reviewed changes

app/src/main/java/org/fossasia/phimpme/utilities/ImgurRetrofitClient.java Outdated Show resolved Hide resolved

iamareebjamal reviewed Oct 19, 2019

View reviewed changes

app/src/main/java/org/fossasia/phimpme/share/SharingActivity.java Outdated Show resolved Hide resolved

codedsun force-pushed the Retrofit branch from 5520a68 to cabdc46 Compare October 19, 2019 19:09

iamareebjamal reviewed Oct 19, 2019

View reviewed changes

app/src/main/java/org/fossasia/phimpme/utilities/RetrofitClient.java Show resolved Hide resolved

codedsun force-pushed the Retrofit branch from cabdc46 to 82c14fa Compare October 20, 2019 07:11

iamareebjamal reviewed Oct 20, 2019

View reviewed changes

codedsun force-pushed the Retrofit branch 2 times, most recently from ad10f3f to 567d530 Compare October 20, 2019 13:10

iamareebjamal reviewed Oct 20, 2019

View reviewed changes

codedsun force-pushed the Retrofit branch from 567d530 to 714eb4f Compare October 20, 2019 15:23

iamareebjamal reviewed Oct 20, 2019

View reviewed changes

codedsun force-pushed the Retrofit branch from 714eb4f to 5f098d7 Compare October 20, 2019 16:41

iamareebjamal reviewed Oct 20, 2019

View reviewed changes

feat: Removed volley and shifted api calls to retrofit

f928395

codedsun force-pushed the Retrofit branch from 5f098d7 to f928395 Compare October 20, 2019 16:54

iamareebjamal approved these changes Oct 20, 2019

View reviewed changes

iamareebjamal merged commit 9d671ac into fossasia:development Oct 21, 2019

	public interface ImgurApiInterface {
	public interface ImgurApi {

	public String IMGUR_HEADER_USER = "Bearer";
	private static final String IMGUR_HEADER_USER = "Bearer";

	public static String IMGUR_HEADER_CLIENT = "Client-ID";
	private static final String IMGUR_HEADER_CLIENT = "Client-ID";

feat: Removed volley and shifted api calls to retrofit #2896

feat: Removed volley and shifted api calls to retrofit #2896

Conversation

codedsun commented Oct 19, 2019

codedsun commented Oct 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codedsun Oct 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codedsun commented Oct 19, 2019 • edited Loading

codedsun commented Oct 19, 2019

iamareebjamal commented Oct 19, 2019 • edited Loading

codedsun commented Oct 19, 2019

codedsun commented Oct 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codedsun commented Oct 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codedsun Oct 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codedsun commented Oct 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codedsun commented Oct 20, 2019

codedsun commented Oct 20, 2019

codedsun Oct 19, 2019 •

edited

Loading

codedsun commented Oct 19, 2019 •

edited

Loading

iamareebjamal commented Oct 19, 2019 •

edited

Loading

codedsun Oct 20, 2019 •

edited

Loading