5
\$\begingroup\$

I have JSON which is coming by executing a URL. I want to store all the value of id field in a List<String> by parsing through GSON. Also, if the next field is true, then that means there is another URL at the bottom of the JSON which I need to use to get the second page which will also look like this JSON response which will have ids again, so I will add those ids into the same list again.

Here is the flow:

  • Execute the URL and get JSON response. After, that check if the next field is true or false in that JSON response.
  • If it is false, then there is only one page so add all those ids into my List<String> and return.
  • If it is true, then there will be another URL at the bottom. Use that URL and add prefix to it and then execute it to get another JSON response and repeat the same thing until next is false and keep adding all ids into the same list.

JSON response:

{
    "status": {
        "number": "100",
        "value": "ok"
    },
    "next": true,
    "result": [{
        "type": "Process",
        "id": "1.2.3.4.5.6",
        "device": "hello"
    }, {
        "type": "Process",
        "id": "9.55.1.2.3.2",
        "device": "world"
    }, {
        "type": "Process",
        "id": "5.4.3.2.1.2",
        "device": "proc"
    }, {
        "type": "Process",
        "id": "99.11.11.2.3.4",
        "device": "oreon"
    }],
    "pagination": {
        "cursor": ["", "57d9e9d4e4b073aa2f401542", ""],
        "limit": [10000, 1000, 1000],
        "hint": 0,
        "maxFetch": 0,
        "skipEmptyPage": false
    },
    "next": {
        "method": "GET",
        "url": "/tries/next_set"
    }
}

Working code:

  public List<String> getIds() {
    List<String> ids = new ArrayList<>();
    try {
      String url = generateUrl();
      boolean hasMore = true;
      while (hasMore) {
        Optional<String> response = executeUrl(url);
        if (!response.isPresent()) {
          JsonElement jelement = new JsonParser().parse(response.get());
          JsonObject jobject = jelement.getAsJsonObject();
          hasMore = jobject.get("next").getAsBoolean();
          if (hasMore) {
            url = PREFIX + jobject.getAsJsonObject("next").get("url").getAsString();
          }
          JsonArray jarray = jobject.getAsJsonArray("result");
          for (JsonElement type : jarray) {
            ids.add(type.getAsJsonObject().get("id").getAsString());
          }
        } else {
          hasMore = false; // to avoid infinite loop if response is not present
        }
      }
    } catch (Exception ex) {
      // log error
    }
    return ids;
  }

Below is how my execute method looks like:

  private Optional<String> execute(final String url) {
    Optional<String> abcResponse = Optional.absent();
    try (AsyncHttpClient asyncHttpClient = new DefaultAsyncHttpClient()) {
      Future<Response> future = asyncHttpClient.prepareGet(url).execute();
      Response response = future.get();
      abcResponse = Optional.of(response.getResponseBody(StandardCharsets.UTF_8.displayName()));
      System.out.println(response.getResponseBody());
    } catch (IOException | InterruptedException | ExecutionException ex) {
      // logging error
    }
    return abcResponse;
  }

The first URL is always generated by calling the generateUrl() method which is different than the subsequent URL which are there in the bottom of the JSON response. I want to see if there is any better way to do the same thing.

\$\endgroup\$
5
\$\begingroup\$

Overall I think it is a good solution.

Exception handling

I think getIds()-method cannot handle exceptions properly in the context of any application. Currently it makes the result look like as nothing has happened even an exception is thrown. I do not think that callers can make valid assumptions on the returned value so I suggest to encapsulate the general Exception into a RuntimeException (or some of your own). The caller of the method should know if the result is valid. If you want the caller continue to process with partial results then you can make your own exception containing the information received so far. But to abstract from errors is dangerous.

The decision of throwing exceptions is as important as catching, rethrowing, wrapping, logging and handle them.

If you throw an exception you do it because you want to indicate that the assumptions a method has are not met so it cannot fulfill its work.

If you want to rethrow an exception the purpose mostly is to intercept information.

If you wrap an exception you want to

  1. map the Exception to an exception of YOUR domain
  2. make the Exception signature irrelevant (RuntimeException)

An exception should be handled at a point it can be handled. And this is not neccessarily the method it occurs.

Logging an exception is the last option for exceptional cases that have no previous mentioned handle. And this is also not neccessarily the method it occurs.

Remove Optional-construct

To exit the loop you determine the existance of a next "url". That should be your single point of truth. "getResponse" is called if "hasMore" is true. But If "getResponse" gets called then it is called under the assumption that there "is" more. If the getResponse-method cannot fulfill the work it is an exception. My view is that getting an "Optional" when "hasMore" has determined before is contradictory.

Throw an exception and the same mechanism as described in "Exception handling" is used. It is transparent to the caller. The caller always will get all necessary information and can decide on its own what to do. Either the getIds()-method returns normal or an exception is thrown that contains the Ids received so far.

Ambigious next-key and semantic redundancy

Currently you have two next-keys holding different values: boolean and String. Theoretically a JsonObject should have unique keys. But any behaviour you rely on to retreive values from this key is specific to the JSON library you use. Maybe GSON does it right somehow.

Furthermore you have semantical redundancy. The existance of the next URL is sufficient information to determine if there is a next URL.

My suggestion is to get rid of the semantic redundancy as early as possible so that further algorithms work with a single source of truth. Maybe you have a preprocession of the Json String and return the JsonObject as the return value of your execute-method().

Reuse JsonParser

I think that is a little issue. But I would reuse the JsonParser-instance as I do not know if this is an expensive operation to instantiate this kind of object.

Extracting constants

There are code fragments that should be extracted to constants. the "key" String is repeated but adresses the same semantic. But also the other keys to access the json object.

Variable scope

The subjects become pedantic... You may reduce the scope of the "hasMore" variable by using a for loop rather than a while loop.

Types

You should also have a look at your GSON-Types. To determine the id you ask an JsonElement. A JsonElement may also be a JsonNull-Object (see here). Maybe you have to introduce some type-check.

Code

private static final String KEY_NEXT = "next";
private static final String KEY_URL = "url";
private static final String KEY_RESULT = "result";
private static final String KEY_ID = "id";


public List<String> getIds() {
    List<String> ids = new ArrayList<>();
    try {
        String url = generateUrl();
        final JsonParser jsonParser = new JsonParser();
        for (boolean hasMore = true; hasMore;) {
            JsonObject jobject = execute(url);
            JsonArray jarray = jobject.getAsJsonArray(KEY_RESULT);
            for (JsonElement type : jarray) {
                ids.add(type.getAsJsonObject().get(KEY_ID).getAsString());
            }
            hasMore = jobject.has(KEY_NEXT); // assumption, that only the next-key with the url is possibly present
            if (hasMore) {
                url = PREFIX + jobject.getAsJsonObject(KEY_NEXT).get(KEY_URL).getAsString();
            }
        }
    } catch (Exception ex) {
        throw new PartialResultException(ex, ids); 
    }
    return ids;
}


private static class PartialResultException extends RuntimeException {

    private List<String> idsReceivedSoFar;

    public PartialResultException(Exception ex, List<String> idsReceivedSoFar) {
        super(ex);
        this.idsReceivedSoFar = idsReceivedSoFar;
    }

    public List<String> getIdsReceivedSoFar() {
        return idsReceivedSoFar;
    }

}


// Example call
public void caller() {

    List<String> ids = new ArrayList<>();

    try {
        ids.addAll(getIds());
    } catch (PartialResultException e) {
        // omit this if the caller is not interested in the ids received so far if an exception occured
        ids.addAll(e.getIdsReceivedSoFar());
    }

    // code that uses the ids
    for (String string : ids) {
        ...
    }

}

The execute-method will either wrap the occuring exceptions to a RuntimeException...

private JsonObject execute(final String url) {
    try (AsyncHttpClient asyncHttpClient = new DefaultAsyncHttpClient()) {
        Future<Response> future = asyncHttpClient.prepareGet(url).execute();
        Response response = future.get();
        final JsonParser jsonParser = new JsonParser();
        JsobObject jsonResponse = jsonParser.parse(response.getResponseBody(StandardCharsets.UTF_8.displayName())).getAsJsonObject();
        removeNextKeyBooleanValue(jsonParser);
        return jsonResponse;
    } catch (IOException | InterruptedException | ExecutionException ex) {
        throw new RuntimeException(ex);
    }
}     

... or you may throw them

private JsonObject execute(final String url) throws IOException, InterruptedException, ExecutionException {
    try (AsyncHttpClient asyncHttpClient = new DefaultAsyncHttpClient()) {
        Future<Response> future = asyncHttpClient.prepareGet(url).execute();
        Response response = future.get();
        final JsonParser jsonParser = new JsonParser();
        JsobObject jsonResponse = jsonParser.parse(response.getResponseBody(StandardCharsets.UTF_8.displayName())).getAsJsonObject();
        removeNextKeyBooleanValue(jsonParser);
        return jsonResponse;
    }
}

The point is that there is no rule to catch and handle an exception where it occurs or as early as possible. The rule is: it has to be the semantic correct location under the assumptions of the involved methods.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the great feedback. In my case, if for whatever reason, I am not able to parse response or execute URL properly, then I am returning empty List back. I guess that's ok in my use case here. Also to execute URL, I am using AsyncHttpClient which executes URL and return whole JSON response back as a String. And since executing URL can give some IOException or `TimeoutException', didn't got a response within a particular time, so that is why I return Optional<String> back as well. \$\endgroup\$ – arsenal Feb 11 '17 at 11:07
  • \$\begingroup\$ I see the caller as the responsible to interprete the exceptional cases and decide if he wants to go on with an empty list or not. \$\endgroup\$ – oopexpert Feb 11 '17 at 11:42
  • \$\begingroup\$ hmmm so what I am supposed to do in PartialResultException constructor? Also I am not sure under what cases NoSuchElementException exception will be thrown. In my case, I can throw IOException since I am executing URL and that is what the exception can get thrown in that method. \$\endgroup\$ – arsenal Feb 11 '17 at 12:03
  • \$\begingroup\$ You negate the two most important annotations as I ordered them by importance. The other things in comparison are trivial while the first two things were very hard to unpack as they have interdependencies and semantic issues. As your comment came while writing this comment I will extend my answer. \$\endgroup\$ – oopexpert Feb 11 '17 at 12:04
  • \$\begingroup\$ I also edited my question to add execute method which executes the URL.. Just to give you some idea how it looks like.. In my case, I am thinking to retry execute method few times if it throws exception. Because I want to get the data back at any cost by executing these url's. So for whatever reason first time, it gave IOException or Timedout for whatever reason, I can try calling execute method few times to make sure it give the response back. \$\endgroup\$ – arsenal Feb 11 '17 at 12:11

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.