2
\$\begingroup\$

I have a Repository class which looks like this:

public class Repository extends Observable {

    private List<Event> events;
    private List<Article> articles;
    private List<Article> sportArticles;
    private List<Article> fitnessArticles;
    private List<Article> governmentArticles;
    private Article mainArticle;
    private Configuration config;


    public Repository() {
        loadRepository(); 
    }

    private void loadRepository() {
        ExecutorService exService = Executors.newSingleThreadExecutor();
        exService.submit(new Runnable() {
            @Override
            public void run() {
                try {
                    OkHttpClient client = new OkHttpClient.Builder().connectTimeout(15, TimeUnit.SECONDS).build();
                    final Gson gson = new GsonBuilder().registerTypeHierarchyAdapter(Calendar.class, new CalendarGson()).create();
                    final AtomicBoolean error = new AtomicBoolean();
                    final CountDownLatch latch = new CountDownLatch(7);
                    Request.Builder requestbuilder = new Request.Builder();
                    Request articlesRequest = requestbuilder.url("https://apiurlarticles").build();
                    Request eventsRequest = requestbuilder.url("https://apiurlevents").build();
                    Request sportArticlesRequest = requestbuilder.url("https://apiurlsports").build();
                    Request fitnessArticlesRequest = requestbuilder.url("https://apiurlfitness").build();
                    Request governmentArticlesrequest = requestbuilder.url("https://apiurlgovernment").build();
                    Request mainArticleRequest = requestbuilder.url("https://apiurlmain").build();
                    Request configurationRequest = requestbuilder.url("https://apiurlconfig").build();

                    //Article Request
                    client.newCall(articlesRequest).enqueue(new Callback() {
                        @Override
                        public void onFailure(Call call, IOException e) {
                            error.set(true);
                            latch.countDown();
                        }
                        @Override
                        public void onResponse(Call call, Response response) {
                            try {
                                ResponseBody body = response.body();
                                if (response.code() != 200 || body == null) {
                                    error.set(true);
                                } else {
                                    articles = gson.fromJson(body.string(), new TypeToken<List<Article>>() {
                                    }.getType());
                                }
                            } catch (IOException | JsonSyntaxException e) {
                                error.set(true);
                            }
                            latch.countDown();
                        }
                    });

                    //Events Request
                    client.newCall(eventsrequest).enqueue(new Callback() {
                        @Override
                        public void onFailure(Call call, IOException e) {
                            error.set(true);
                            latch.countDown();
                        }
                        @Override
                        public void onResponse(Call call, Response response) {
                            try {
                                ResponseBody body = response.body();
                                if (response.code() != 200 || body == null) {
                                    error.set(true);
                                } else {
                                    events = gson.fromJson(body.string(), new TypeToken<List<Event>>() {
                                    }.getType());
                                }
                            } catch (IOException | JsonSyntaxException e) {
                                error.set(true);
                            }
                            latch.countDown();
                        }
                    });

                    //Sports Request
                    client.newCall(sportArticlesRequest).enqueue(new Callback() {
                        @Override
                        public void onFailure(Call call, IOException e) {
                            error.set(true);
                            latch.countDown();
                        }
                        @Override
                        public void onResponse(Call call, Response response) {
                            try {
                                ResponseBody body = response.body();
                                if (response.code() != 200 || body == null) {
                                    error.set(true);
                                } else {
                                    sportArticles = gson.fromJson(body.string(), new TypeToken<List<Article>>() {
                                    }.getType());
                                }
                            } catch (IOException | JsonSyntaxException e) {
                                error.set(true);
                            }
                            latch.countDown();
                        }
                    });

                    //Fitness Request
                    client.newCall(fitnessArticlesRequest).enqueue(new Callback() {
                        @Override
                        public void onFailure(Call call, IOException e) {
                            error.set(true);
                            latch.countDown();
                        }
                        @Override
                        public void onResponse(Call call, Response response) {
                            try {
                                ResponseBody body = response.body();
                                if (response.code() != 200 || body == null) {
                                    error.set(true);
                                } else {
                                    fitnessArticles = gson.fromJson(body.string(), new TypeToken<List<Article>>() {
                                    }.getType());
                                }
                            } catch (IOException | JsonSyntaxException e) {
                                error.set(true);
                            }
                            latch.countDown();
                        }
                    });

                    //Government Request
                    client.newCall(governmentArticlesrequest).enqueue(new Callback() {
                        @Override
                        public void onFailure(Call call, IOException e) {
                            error.set(true);
                            latch.countDown();
                        }
                        @Override
                        public void onResponse(Call call, Response response) {
                            try {
                                ResponseBody body = response.body();
                                if (response.code() != 200 || body == null) {
                                    error.set(true);
                                } else {
                                    governmentArticles = gson.fromJson(body.string(), new TypeToken<List<Article>>() {
                                    }.getType());
                                }
                            } catch (IOException | JsonSyntaxException e) {
                                error.set(true);
                            }
                            latch.countDown();
                        }
                    });

                    //Main Article Request
                    client.newCall(mainArticleRequest).enqueue(new Callback() {
                        @Override
                        public void onFailure(Call call, IOException e) {
                            error.set(true);
                            latch.countDown();
                        }
                        @Override
                        public void onResponse(Call call, Response response) {
                            try {
                                ResponseBody body = response.body();
                                if (response.code() != 200 || body == null) {
                                    error.set(true);
                                } else {
                                    mainArticle = gson.fromJson(body.string(), new TypeToken<Article>() {
                                    }.getType());
                                }
                            } catch (IOException | JsonSyntaxException e) {
                                error.set(true);
                            }
                            latch.countDown();
                        }
                    });

                    //Configuration request
                    client.newCall(configurationRequest).enqueue(new Callback() {
                        @Override
                        public void onFailure(Call call, IOException e) {
                            error.set(true);
                            latch.countDown();
                        }
                        @Override
                        public void onResponse(Call call, Response response) {
                            try {
                                ResponseBody body = response.body();
                                if (response.code() != 200 || body == null) {
                                    error.set(true);
                                } else {
                                    config = gson.fromJson(body.string(), new TypeToken<Configuration>() {
                                    }.getType());
                                }
                            } catch (IOException | JsonSyntaxException e) {
                                error.set(true);
                            }
                            latch.countDown();
                        }
                    });
                    try {
                        latch.await();
                    } catch (InterruptedException e) {
                        error.set(true);
                    }
                    notifyObservers(error.get() ? HTTPRequestStatus.HTTPERROR : HTTPRequestStatus.OK);
                } catch (KeyManagementException | NoSuchAlgorithmException | KeyStoreException e) {
                    notifyObservers(HTTPRequestStatus.TLSERROR);
                }
            }
        });
        exService.shutdown();
    }
}

This code calls several HTTP requests asynchronously and wait for all of them to conclude. If any request fails it notifies with an error, otherwise success. These requests returns JSON and I parse them using Gson.

For simplicity I have only showed two requests, however I want to refactor this code for any number of requests. There is a lot of similar code, but I can't figure a way to refactor it. I have already tried to create a custom Callback class however I can't figure a way to pass a Type and return the parsed value.

How would you approach this one?

\$\endgroup\$
  • 2
    \$\begingroup\$ Simplicity isn't usually the way to go here. I recommend you show the rest of your code. You might get different answers based on the less "simple" code than you would with what you currently have. Also please change the title to describe What your code Does as apposed to your goal. (Almost everyone wants to refactor their code.) \$\endgroup\$ – bruglesco Dec 27 '18 at 19:36
  • \$\begingroup\$ @bruglesco The difference between this code and the actual one is that there are like 10 Request Objects and 10 client.newCalls(request) , should I include them anyway? \$\endgroup\$ – Exprove Dec 27 '18 at 20:55
  • \$\begingroup\$ Yes. You should. \$\endgroup\$ – bruglesco Dec 27 '18 at 21:17
3
\$\begingroup\$

Welcome to Code Review!

Thanks for sharing your code.

As you observed the main problem is code duplication.

We have basically two options to solve that problem. Both of them use the basic approach to separate the common code from the different code and "inject" the differing code into the common code.

prepare injecting differing code

When looking at your code the only difference between the //Article Request and the //Events Request are these lines:

//Article Request
articles = gson.fromJson(body.string(), new TypeToken<List<Article>>() {
                                    }.getType());

//Events Request 
events = gson.fromJson(body.string(), new TypeToken<List<Event>>() {
                                    }.getType());

One problem here is that you change the objects pointed to by the member variables articles and events. We could make is easier to change if the objects would be reused:

//Article Request
articles.clear();
articles.addAll(gson.fromJson(body.string(), new TypeToken<List<Article>>() {
                                    }.getType()));

//Events Request 
events.clear();
events.addAll(gson.fromJson(body.string(), new TypeToken<List<Event>>() {
                                    }.getType()));

now we can extract one of that parts to a new Method using the IDEs automated refactoring "extract method":

private void deserializeJson(
      List<Event> events,
      Gson gson,
      ResponseBody body
){
    events.clear();
    events.addAll(gson.fromJson(body.string(), new TypeToken<List<Event>>() {
                                        }.getType()));
} 

//Events Request 
// ...
} else {
      deserializeJson((List<Event>)events,
      Gson gson,
      ResponseBody body);
}

Next we have to mat this new private method "generic":

private <L extends List> void deserializeJson(
      L events,
      Gson gson,
      ResponseBody body
){
    events.clear();
    events.addAll(gson.fromJson(body.string(), new TypeToken<L>() {
                                        }.getType()));
} 

Now we can change toe other place too:

//Articles  Request
// ...
} else {
      deserializeJson((List<Article>)articles);
}

from here we have two options:

  1. consolidating the common code in a single generic typed method.
  2. puttig the difering code into an "specialized classes" providing the differing behavior via an common interface.

common code in a single generic typed method

We basically do he same as above: we extract the //Events Request section or the //Articles Request into a new private method and make it "generic"

private <L extends List> void deserializeFromJson(
    L theList, 
    OkHttpClient client, 
    Gson gson 
){
    client.newCall(articlesrequest).enqueue(new Callback() {
        @Override
        public void onFailure(Call call, IOException e) {
            error.set(true);
            latch.countDown();
        }
        @Override
        public void onResponse(Call call, Response response) {
            try {
                ResponseBody body = response.body();
                if (response.code() != 200 || body == null) {
                    error.set(true);
                } else {
                     deserializeJson((L)theList, gson, body);
                }
            } catch (IOException | JsonSyntaxException e) {
                error.set(true);
            }
            latch.countDown();
        }
    });
}


//Article Request
deserializeFromJson(
   (List<Articles>)articles,
   client,
   gson
);

//Events Request
client.newCall(eventsrequest).enqueue(new Callback() {
  // ...

and then also replace the other part:

//Article Request
deserializeFromJson(
   (List<Articles>)articles,
   client,
   gson
);

//Events Request
deserializeFromJson(
   (List<Event>)events,
   client,
   gson
);

creating specialized classes with interface

for this we have to move the private method created in the first section into a new inner class

class your current class:

  private static class JsonDeserializer<T> {

     void deserialize(
          List<T> theList,
          Gson gson,
          ResponseBody body
     ){
        theList.clear();
        theList.addAll(gson.fromJson(body.string(), new TypeToken<List<T>>() {
                                            }.getType()));

     }
  }
  // ...

    //Articles  Request
    // ...
    } else {
          new JsonDeserializer<Article>().deserialize(articles, gson, body);
    }
    //...

Since articles and events are member variables I'd rather have them as constructor parameters in the new class:

class your current class:

  private static class JsonDeserializer<T> {
     private final List<T> theList;
     JsonDeserializer( List<T> theList){
       this.theList = theList;
     }

     void deserialize(
          Gson gson,
          ResponseBody body
     ){
        theList.clear();
        theList.addAll(gson.fromJson(body.string(), new TypeToken<List<T>>() {
                                            }.getType()));

     }
  }
  // ...

    //Articles  Request
    // ...
    } else {
          new JsonDeserializer<Article>(articles).deserialize(gson, body);
    }
    //...
    //Event  Request
    // ...
    } else {
          new JsonDeserializer<Event>(eventss).deserialize(gson, body);
    }
    //...

Now we can create the JsonDeserializer instances at the top of the method:

private void loadRepository() {
    JsonDeserializer<Article> articleDeserializer =  new JsonDeserializer<>(articles);
    JsonDeserializer<Event> eventDeserializer =  new JsonDeserializer<>(event);
    ExecutorService exService = Executors.newSingleThreadExecutor();
    // ...
    //Articles  Request
    // ...
    } else {
          articleDeserializer.deserialize(gson, body);
    }
    //...
    //Event  Request
    // ...
    } else {
          eventDeserializer.deserialize(gson, body);
    }
    //...

Now the only difference left in //Articles Request section and //Event Request section is the name of the variable. So we can put the two JsonDeserializer instances into a collection and apply the *common code * as a loop:

private void loadRepository() {
    JsonDeserializer<?> deserializers = Arrays.asList( 
         new JsonDeserializer<Article>(articles),
         new JsonDeserializer<Event>(event)
    );
    ExecutorService exService = Executors.newSingleThreadExecutor();
// ...

    Request eventsrequest = requestbuilder.url("https://apiurlevents").build();

    //Article Request changed to loop
    for(JsonDeserializer<?>  deserializer: deserializers){
        client.newCall(articlesrequest).enqueue(new Callback() {
            @Override
            public void onFailure(Call call, IOException e) {
                error.set(true);
                latch.countDown();
            }
            @Override
            public void onResponse(Call call, Response response) {
                try {
                    ResponseBody body = response.body();
                    if (response.code() != 200 || body == null) {
                        error.set(true);
                    } else {
                        deserializer.deserialize(gson, body);
                    }
                } catch (IOException | JsonSyntaxException e) {
                    error.set(true);
                }
                latch.countDown();
            }
        });
    }
    try {
        latch.await();
    } catch (InterruptedException e) {
        error.set(true);
    }

New document types are just new instances in the collection.

\$\endgroup\$
  • \$\begingroup\$ Really appreciate your help. That refactor would work, however not all my class member variables are from type List. Added more requests examples. If I replace List<T> to <T> would that work? Ofc I would declare JsonDeserializer as <List<Article>> for example. \$\endgroup\$ – Exprove Dec 28 '18 at 0:12
  • \$\begingroup\$ @Exprove however not all my class member variables are from type List I'd create another JsonDeserializer that deserializes the generic type directly. Problem might be to get the de-serialized object out of the JsonDeserializer instance. replacing the List parameter of the constructor with a Listener might work. \$\endgroup\$ – Timothy Truckle Dec 28 '18 at 0:20
2
\$\begingroup\$

You could use a CompletableFuture for this. Make each call return a completed future with the body, or an exceptionally completed future with an error:

private CompletableFuture<String> call(String url) {
    CompletableFuture<String> future = new CompletableFuture<>();

    OkHttpClient client = new OkHttpClient.Builder().connectTimeout(15, TimeUnit.SECONDS).build();
    client.newCall(requestbuilder.url(url).build()).enqueue(new Callback() {
        @Override
        public void onFailure(Call call, IOException e) {
            future.completeExceptionally(e);
        }

        @Override
        public void onResponse(Call call, Response response) {
            try {
                ResponseBody body = response.body();
                if (response.code() != 200 || body == null) {
                    throw new IOException("Http error");
                } else {
                    future.complete(body.string());
                }
            } catch (IOException | JsonSyntaxException e) {
                future.completeExceptionally(e);
            }
        }
    });

    return future;
}

Then add a generic method to make the calls and deserialize the results:

private <T> Future<T> callAndDeserialize(String url, Gson gson, TypeToken<T> typeToken) {
    CompletableFuture<String> future = call(url);
    return future.thenApply(new Function<String, T>() {
        public T apply(String body) {
            return gson.fromJson(body, typeToken.getType()));
        }
    });
}

The loadRepository code would then be something like:

final Gson gson = new GsonBuilder().registerTypeHierarchyAdapter(Calendar.class, new CalendarGson()).create();

Future<List<Article>> articlesFuture = callAndDeserialize("https://apiurlarticles", gson, new TypeToken<List<Article>>() {});
Future<List<Event>> eventsFuture = callAndDeserialize("https://apiurlarticles", gson, new TypeToken<List<Event>>() {});
Future<List<Article>> sportsFuture = callAndDeserialize("https://apiurlarticles", gson, new TypeToken<List<Article>>() {});
Future<List<Article>> fitnessFuture = callAndDeserialize("https://apiurlarticles", gson, new TypeToken<List<Article>>() {});
Future<List<Article>> governmentFuture = callAndDeserialize("https://apiurlarticles", gson, new TypeToken<List<Article>>() {});
Future<Article> mainArticleFuture = callAndDeserialize("https://apiurlarticles", gson, new TypeToken<Article>() {});
Future<Configuration> configurationFuture = callAndDeserialize("https://apiurlarticles", gson, new TypeToken<Configuration>() {});

try {
    articles = articlesFuture.get();
    events = eventsFuture.get();
    sportArticles = sportsFuture.get();
    fitnessArticles = fitnessFuture.get();
    governmentArticles = governmentFuture.get();
    mainArticle = mainArticleFuture.get();
    config = configurationFuture.get();

    notifyObservers(HTTPRequestStatus.OK);
} catch (ExecutionException e) {
    if(e.getCause() instanceof KeyManagementException || e.getCause() instanceof NoSuchAlgorithmException || e.getCause() instanceof KeyStoreException) {
        notifyObservers(HTTPRequestStatus.TLSERROR);
    } else {
        notifyObservers(HTTPRequestStatus.HTTPERROR);
    }
}

The ExecutionException at the end now retains the exception messages, causes and stacktraces. In case there's any unexpected errors you need to debug, you can also log this exception as well notifying the observers.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.