Serializing data from two URLs in the same object efficiently

Question

This is a follow-up to: Serializing JSON data coming from two URLs in the same object

I have two URLs (urlA and urlB) and they both give me a JSON response back in the same JSON format. Below is my JSON string which I am getting back by calling from urlA. I have shortened it down by having only three reportRecords for understanding purposes. In general, it might have more than ~500 reportRecords.

{
   "aggRecords": {
      "reportRecords": [
         {
            "min": 0,
            "max": 12,
            "avg": 0.3699187,
            "count": 246,
            "sumSq": 571,
            "stddev": 1.4779372,
            "median": 0,
            "percentileMap": {
               "95": 4
            },
            "metricName": "TransactionDuration",
            "dimensions": {
               "env": "dev",
               "pool": "titan",
               "Name": "PostProcessing",
               "Type": "PostProcessing"
            },
            "value": 91
         },
         {
            "min": 0,
            "max": 23,
            "avg": 2.3991289E-4,
            "count": 1463031,
            "sumSq": 3071,
            "stddev": 0.045814946,
            "median": 0,
            "percentileMap": {
               "95": 0
            },
            "metricName": "TransactionDuration",
            "dimensions": {
               "env": "dev",
               "pool": "titan",
               "Name": "ResourceContext",
               "Type": "ResourceContext"
            },
            "value": 351
         },
         {
            "min": 0,
            "max": 1209,
            "avg": 1.9203402,
            "count": 7344636,
            "sumSq": 71832774,
            "stddev": 2.4683187,
            "median": 2,
            "percentileMap": {
               "95": 4
            },
            "metricName": "TransactionDuration",
            "dimensions": {
               "env": "dev",
               "pool": "titan",
               "Name": "DataSync",
               "Type": "DataSync"
            },
            "value": 14104200
         }
      ]
   }
}

Similarly I am also getting another JSON response back by calling urlB and it is in the exact same JSON format as shown above, but the values for all the keys are different.

Now what I need to do is, I need to serialize JSON response coming from both the URLs (urlA and urlB) in DataTransactionMetrics object only and then iterate DataTransactionMetrics object and extract relevant information from it in which I am interested in it. Meaning, I will be extracting few Name values from the DataTransactionMetrics object in which I am interested in it and populate it into hostMetricsList object and at the end print out the full list.

Below is my full code which does the JSON serialization of both the URL's -

private static final List<String> hostNames = Arrays.asList("hostA", "hostB", "hostC"); 

public static void main(String[] args) throws IOException {
    List<HostMetrics> hostMetricsList = new ArrayList<HostMetrics>();
    for (String hostName : hostNames) {
        Calendar startDate = new GregorianCalendar();
        startDate.set(Calendar.MINUTE, 0);
        Calendar endDate = (Calendar) startDate.clone();
        startDate.set(Calendar.HOUR_OF_DAY, 0);

        HostMetrics hostMetrics = new HostMetrics(hostName, startDate, endDate);
        hostMetricsList.add(hostMetrics);
    }

    // and then print out the list
    System.out.println(hostMetricsList);
}

And below is my DataTransactionMetrics class -

public class DataTransactionMetrics {

    private String metricName;
    private Map<String, Integer> percentileMap;
    private String median;
    private String stddev;
    private String sumSq;
    private String count;
    private String avg;
    private String max;
    private String min;

    // getters here

    public Dimensions dimensions;

    class Dimensions {
        private String env;
        private String pool;
        @SerializedName("Name")
        private String name;

        // getters here
    }
}

And below is my HostMetrics class which has all the logic. Can we do some improvements here?

public class HostMetrics {

    private String hostName;
    private String totalNumberOfSyncCall;
    private String syncNinetyFivePercentileCall;
    private String syncAvgCall;
    private String syncMaxCall;
    private String totalNumberOfAsyncCall;
    private String asyncNinetyFivePercentileCall;
    private String asyncAvgCall;

    private static final DateFormat df = new SimpleDateFormat("yyyy/MM/dd HH:mm");
    private static RestTemplate restTemplate = new RestTemplate();
    private static final Gson gson = new Gson();
    private static final Type listOfMetricsType = new TypeToken<List<DataTransactionMetrics>>() {}.getType();
    private static final String BASE_URL = "http://data.host.com/reports/";
    private static final String TRANSACTION_METRIC = "/Transaction?defaultDim=Name&defaultDim=Type&metric=TransactionDuration&percentile=95";
    private static final String EVENT_METRIC = "/Event?defaultDim=Name&defaultDim=Type&metric=TotalCount&percentile=95";

    public HostMetrics(String hostname, Calendar startDate, Calendar endDate) {
        constructURL(hostname, startDate, endDate);
    }

    private void constructURL(String hostname, Calendar startDate, Calendar endDate) {
        String startTime = df.format(startDate.getTime());
        String endTime = df.format(endDate.getTime());

        // construct urlA
        String urlA = BASE_URL + hostName + TRANSACTION_METRIC + "&startTime=" + startTime
                + "&endTime=" + endTime;

        // construct urlB
        String urlB = BASE_URL + hostName + EVENT_METRIC + "&startTime=" + startTime
                + "&endTime=" + endTime;

        // serialize everything in DataTransactionMetrics object, both the URL's
        List<DataTransactionMetrics> dataMetrics = loadMetrics(urlA);
        dataMetrics.addAll(loadMetrics(urlB));

        hostName = hostname;

        for (DataTransactionMetrics metrics : clientMetrics) {
            if (metrics.getDimensions().getName().equalsIgnoreCase("DataSync")) {
                totalNumberOfSyncCall = metrics.getCount();
                syncNinetyFivePercentileCall = String.valueOf(metrics.getPercentileMap().get("95"));
                syncAvgCall = metrics.getAvg();
                syncMaxCall = metrics.getMax();
            } else if (metrics.getDimensions().getName().equalsIgnoreCase("DataASync")) {
                totalNumberOfAsyncCall = metrics.getCount();
                asyncNinetyFivePercentileCall = String.valueOf(metrics.getPercentileMap().get("95"));
                asyncAvgCall = metrics.getAvg();
            }
        }
    }

    private static List<DataTransactionMetrics> loadMetrics(String url) {
        String jsonString = restTemplate.getForObject(url, String.class);

        JsonObject json = new JsonParser().parse(jsonString).getAsJsonObject();
        JsonArray jarr = json.getAsJsonObject("aggRecords").getAsJsonArray("reportRecords");

        return gson.fromJson(jarr, listOfMetricsType);
    }

    // toString method here
}   

Now I have got everything working in the above code. I am opting for code review to see whether we can improve anything here in the code.

Answer 1

The main method is doing too much. It would be better to extract its content to a method, with a meaningful name.

---

I don't really see the point of this:

List<HostMetrics> hostMetricsList = new ArrayList<HostMetrics>();
for (String hostName : hostNames) {
    // ...
    HostMetrics hostMetrics = new HostMetrics(hostName, startDate, endDate);
    hostMetricsList.add(hostMetrics);
}
System.out.println(hostMetricsList);

Do you really want to print out the HostMetrics as a list? Printing them one by one is not good enough?

for (String hostName : hostNames) {
    // ...
    HostMetrics hostMetrics = new HostMetrics(hostName, startDate, endDate);
    System.out.println(hostMetrics);
}

---

I don't know of a rule against cloning and passing around Calendar objects, but it just doesn't seem natural. Instead of this:

Calendar startDate = new GregorianCalendar();
startDate.set(Calendar.MINUTE, 0);
Calendar endDate = (Calendar) startDate.clone();
startDate.set(Calendar.HOUR_OF_DAY, 0);
HostMetrics hostMetrics = new HostMetrics(hostName, startDate, endDate);

I would do like this:

Calendar cal = new GregorianCalendar();
cal.set(Calendar.MINUTE, 0);
Date startDate = cal.getTime();
cal.set(Calendar.HOUR_OF_DAY, 0);
Date endDate = cal.getTime();

HostMetrics hostMetrics = new HostMetrics(hostName, startDate, endDate);

This means of course that you'll have to change the HostMetrics constructor to take Date params instead of Calendar, but I think that's a good thing.

---

Here, appending the startTime and endTime parameters is duplicated:

String urlA = BASE_URL + hostName + TRANSACTION_METRIC + "&startTime=" + startTime
        + "&endTime=" + endTime;

String urlB = BASE_URL + hostName + EVENT_METRIC + "&startTime=" + startTime
        + "&endTime=" + endTime;

I would extract the duplicated parts to a temp variable:

String extraParams = "&startTime=" + startTime + "&endTime=" + endTime;
String urlA = BASE_URL + hostName + TRANSACTION_METRIC + extraParams;
String urlB = BASE_URL + hostName + EVENT_METRIC + extraParams;

---

Sometimes you call a hostname variable as hostname, sometimes hostName. One day you might mix them up. I recommend to use hostname everywhere consistently.

Answer 2

java.text.SimpleDateFormat is not thread-safe:

Synchronization

Date formats are not synchronized. It is recommended to create separate format instances for each thread. If multiple threads access a format concurrently, it must be synchronized externally.

Consider using ThreadLocal<SimpleDateFormat> instead. Or, as you are working with Calendar instances, you may find JodaTime useful in your efforts.

---

The name of constructURL is misleading; while it does construct URLs, among other things, it also loads network resources. Consider splitting the responsibilities:

class HostMetricLoader {
  public HostMetrics loadMetrics(String hostname, Calendar startDate, Calendar endDate) {
    final List<DataTransactionMetrics> metrics = new ArrayList<>();
    metrics.addAll(loadMetrics(getUrl(EVENT, hostname, startDate, endDate)));
    metrics.addAll(loadMetrics(getUrl(TRANSACTION, hostname, startDate, endDate)));

    final HostMetrics hostMetrics = new HostMetrics(hostname, startDate, endDate);
    for ( final DataTransactionMetrics data : metrics ) {
      hostMetrics.addData(data);
    }

    return metrics;
  }

  protected String getURL(MetricType type, String hostname, Calendar startDate, Calendar endDate) {
    return String.format(/*...*/);
  }

  protected List<DataTransactionMetrics> loadMetrics(String url) {
    // ...
  }
}

enum MetricType {
  EVENT,
  TRANSACTION;
}

class HostMetrics {
  // fields
  // ...

  public void addData(DataTransactionMetrics data) {
    // if this gets larger, split into two functions
    if (metrics.getDimensions().getName().equalsIgnoreCase("DataSync")) {
      totalNumberOfSyncCall = metrics.getCount();
      syncNinetyFivePercentileCall = String.valueOf(metrics.getPercentileMap().get("95"));
      syncAvgCall = metrics.getAvg();
      syncMaxCall = metrics.getMax();
    } else if (metrics.getDimensions().getName().equalsIgnoreCase("DataASync")) {
      totalNumberOfAsyncCall = metrics.getCount();
      asyncNinetyFivePercentileCall = String.valueOf(metrics.getPercentileMap().get("95"));
      asyncAvgCall = metrics.getAvg();
    }
  }
}