Parse the response from http call and populate multiple maps

Question

I have to parse a response after making HTTP call on the server. If the response is not successful then try another server otherwise parse the successful response and populate two ConcurrentHashMap and break out of for loop. Also all the servers will give same exact response.

Below is how my response will look like after executing an URL on any of the server. Each line in the response separated by new line (\n).

11.111.101.203,1,2,9096,4,process_1234
11.111.101.204,1,2,9096,4,process_7110
11.111.101.205,2,2,9096,4,process_7890
11.111.101.206,2,2,9096,4,process_1110
11.124.182.109,1,1,29000,4,client_ty129
12.265.200.100,4,1,29000,4,client_oiu654
13.451.453.1136,3,625,20002,2,tr_old_dev
15.223.114.111,3,608,20002,2,jh7_new_laptop
29.198.223.265,2,441,9094,1,a3_new_proc
11.111.101.203,1,5,33000,2,device_09876asd
11.111.101.204,2,7,33000,2,device_kjuy6543

Below are the types of category we can have in the above response and each line will always be in that same format. If by any chance it is not, then we can throw an exception.

• First: A line which will have "process_"
• Second: A line which will have "client_"
• Third: A line which will have "device_"
• Fourth: A line which does not have all above three.

Now I need to populate two maps basis on above response:

• If a line that follows fourth category condition above, I will split the line on "," and get 0 index which is "ip address" and second index which is "appId" and populate it in a "ipAddressToAppidMapping" map. For example in below line:

  13.451.453.1136,3,625,20002,2,tr_old_dev

  ipAddressToAppidMapping map will have 13.451.453.1136 as key and value as 625 and I will do same things for other lines which follows same pattern.

• If a line that follows first category condition, I will again split the line on "," and and get 0 index which is "ip address" and first index which is "datacenter number" and third index which is "portNumber" and fourth index which is "numberOfPorts". Now basis on "datacenter number", I will locate actual enum for that from Datacenter enum class which will be my ipAddressToTcpAddress map key. Now I have to make a List<String> which will be value for my map given a "numberOfPorts", "portNumber" and "ip address". For example for below line:

  11.111.101.203,1,2,9096,4,process_1234

  My list will look like this after using "numberOfPorts as 4":

  [tcp://11.111.101.203:9099, tcp://11.111.101.203:9098, tcp://11.111.101.203:9097]
  

  As you can see it will not contain first port number which is 9096 so it will have 9097,9098,9099 and it will skip first port whatever is there on the line and use remaining "numberOfPorts -1" always.

Below is my code where loadConfig method will be called by a background thread every 15 minutes to populate these two maps and then from multiple threads, I will access these maps through getters to get data from them so it has to be thread safe and that's why I am using ConcurrentHashMap.

  private static final Splitter SPLITTER = Splitter.on(',').trimResults().omitEmptyStrings();
  private final Map<String, Short> ipAddressToAppidMapping = new ConcurrentHashMap<>();
  private final Map<DatacenterEnum, List<String>> ipAddressToTcpAddress =  new ConcurrentHashMap<>();

  private void loadConfig() {
    List<String> servers = getServers();
    for (String server : servers) {
      try {
        String response = HttpClient.getInstance().execute(makeUrl(server));
        if (Strings.isNullOrEmpty(response) || response.equalsIgnoreCase("TIMED OUT")
            || response.equalsIgnoreCase("NO DATA FOUND")) {
          continue;
        }
        parseConfig(response);
        break;
      } catch (Exception ex) {
        logger.logError("error= ", ExceptionUtils.getStackTrace(ex), ", server= ", server);
      }
    }
  }

  private void parseConfig(final String response) throws IOException {
    List<String> lines = IOUtils.readLines(new StringReader(response));
    for (String line : lines) {
      if (line.trim().length() > 0 && !line.startsWith("#") && !line.contains("process_")
          && !line.contains("client_") && !line.contains("device_")) {
        List<String> config = SPLITTER.splitToList(line);
        String ipAddress = config.get(0);
        Short appId = Short.parseShort(config.get(2));
        ipAddressToAppidMapping.put(ipAddress, appId); // populate first map
      } else if (line.trim().length() > 0 && !line.startsWith("#") && line.contains("process_")) {
        List<String> config = SPLITTER.splitToList(line);
        String ipAddress = config.get(0);
        int datacenter = Integer.parseInt(config.get(1));
        int portNumber = Integer.parseInt(config.get(3));
        int numberOfPorts = Integer.parseInt(config.get(4));
        DatacenterEnum dc = Utils.isProduction() ? DatacenterEnum.name((byte) datacenter) : DatacenterEnum.DEV;
        List<String> address = makeTcpAddress(ipAddress, dc, portNumber, numberOfPorts);
        ipAddressToTcpAddress.put(dc, address); // populate second map
      }
    }
  }

  private List<String> makeTcpAddress(String key, DatacenterEnum dc, int portNumber, int numberOfPorts) {
    List<String> address = ipAddressToTcpAddress.get(dc);
    if (CollectionUtils.isEmpty(address)) {
      address = new ArrayList<>();
    }
    for (int num = (portNumber - 1) + numberOfPorts; num >= portNumber + 1; num--) {
      address.add("tcp://" + key + ":" + num);
    }
    return address;
  }

If the code is not running in production, then my key is always DatacenterEnum.DEV for ipAddressToTcpAddress map otherwise it will be always whatever datacenter is there in the line.. And basis on that I keep appending tcpaddress in a List. Also I have access to guava library in my project so I am using it here to split the line on comma. May be I can use same thing to split the response on a new line instead of using IOUtils.readLines.

I have my above code working fine. Wanted to see if there is any better way to do this efficiently so opting for code review.

Answer 1

Production and Development

One very important thing is that you should not have code fragments with that you are able to determine if you are on production or another stage of your deployment chain. (Utils.isProduction())

Of course you want to mock some things on development machines. But that should be done with other mechanisms than within your code. Your build system is one authority to load with such environment decision tasks. Spring also provides basic mechanisms to change behaviour by configuration.

loadConfig

Semantical representants

Try to identify the semantical representants participating to solve the problem and introduce (minimal) elements to express the intention of the code.

Yes you can say if something equals the string "TIMED OUT" then you do X. If you extract this expression into a method you can make it more explicit and say if the response "hasTimedOut" then do X.

There are some other fragments that can also be extracted into separate methods.

Another point is that the code is not explicit expressing the core intention to loop through the servers until the first valid response. This is encoded within the locations of continue and break. A developer who does not know the the business rule while he was told that "loadConfig" does not the right thing (as a thought experiment) he especially has a hard time to evaluate if the "break" was used intentionally.

Avoid continue and break

Here we have an interesting constellation where you potentially hinder the loop to be breaked with a conditional continue before.

Avoid those constructs as they are hard to refactor as the requirements change. You have a hard day to apply the refactoring "extract method" on a method that contains breaks, continues and even multiple return statements.

Code

private void loadConfig() {

    boolean firstValidResponse = false;

    Iterator<String> serverIterator = getServers().iterator();

    while (!firstValidResponse && serverIterator.hasNext()) {

        try {

            String response = requestConfig(server);

            if (hasResponded(response) && !hasTimedOut(response) && hasFoundData(response) { 
                parseConfig(response);
                firstValidResponse = true;
            }

        } catch (Exception ex) {

            handleLoadConfigException(ex, server);

        }

    }

}

parseConfig

Semantical representants

I guess that '#' is something like a comment and should be ignored during the parsing process. Here again you have potential to extract code to deparate methods. One other thing could be "isEmpty(line)".

Token

If you always have 6 tokens in each line OR the category is always encoded in the last token then you should do the split first and check this token on its own. This is alot more explicit as you do not implicitly assume anymore that the categories "process", "client" or "device" can occur in any token (that would be wrong I guess).

Furthermore your token should not be check by "contains". Here also I would go more explicitly changing from "contains" to "startsWith" to assume that the category is always encoded as the first element in the token.

DataCenterEnum

I generally suggest to avoid language structure information within business names. I do not know how it will help somebody to know if it is an enum he is dealing with.

SRP

The method can be splitted to iterate over all lines and process one line. You will apply the single responsibility principle.

Magic numbers

Extract magic numbers as constants with meaningful names. Especially the the indices of the entries in your splitted list should be named properly.

Code

private static final int INDEX_IP_ADDRESS = 0;
private static final int INDEX_DATACENTER = 1;
private static final int INDEX_PORT_NUMBER = 3;
private static final int INDEX_NUMBER_OF_PORTS = 4;
private static final int INDEX_SOURCE = 5;

private void parseConfig(final String response) throws IOException {

    List<String> lines = IOUtils.readLines(new StringReader(response));

    for (String line : lines) {
        if (!isEmpty(line) && !isComment(line)) {
            parseLine(line);
        }
    }

}

private void parseLine(String line) {

    List<String> config = SPLITTER.splitToList(line);

    String source = config.get(INDEX_SOURCE);

    if (isAppEntry(source)) {

        String ipAddress = config.get(INDEX_IP_ADDRESS);
        Short  appId     = Short.parseShort(config.get(2));

        ipAddressToAppidMapping.put(ipAddress, appId); // populate first map

    } else if (isDataCenterEntry(source)) {

        String     ipAddress      = config.get(INDEX_IP_ADDRESS);
        int        portNumber     = Integer.parseInt(config.get(INDEX_PORT_NUMBER));
        int        numberOfPorts  = Integer.parseInt(config.get(INDEX_NUMBER_OF_PORTS));
        Datacenter dataCenter     = Datacenter.name((byte) Integer.parseInt(config.get(INDEX_DATACENTER)));
        List<String> address      = makeTcpAddress(ipAddress, dataCenter, portNumber, numberOfPorts);

        ipAddressToTcpAddress.put(dataCenter, address); // populate second map

    }

}

private boolean isDataCenterEntry(String source) {
    return source.startsWith("process");
}

private boolean isAppEntry(String source) {
    return !isDataCenterEntry(source) && !source.startsWith("client") && !source.startsWith("device");
}

private boolean isComment(String line) {
    return line.startsWith("#");
}

private boolean isEmpty(String line) {
    return line.trim().length() == 0;
}

Further improvements

1. Maybe you find a better name for such generics like "line" or "lines"
2. The categories "client", "process" and "device" could also be held in an enum. That would express those categories as central elements of your current algorithm and maybe future algorithms.