Parsing a string with named sections and a key-value pair on each line

Question

I have a response String as shown below which I need to parse it and store it my class. Format is shown below:

• task name followed by this dotted line ------------- which is also fixed
• and then just below key:value pair. It can have many key value pair

Below is the response string.

abc-------------
Load:79008
Peak:4932152

def-------------
Load:79008
Peak:4932216

ghi-------------
Load:79008
Peak:4874588

pqr-------------
Load:79008
Peak:4874748

Below is my class:

public class NameMetrics {

    private String name;
    private Map<String, String> metrics;

    // setters and getters

}

In the above class, name should be abc and metrics map should have Load as the key and 79008 as the value and same with other key:value pairs. Below is the code I have.

String response = restTemplate.getForObject(url, String.class);
BufferedReader reader = new BufferedReader(new StringReader(response));
NameMetrics current = null;
Map<String, String> metricsHolder = null;
List<NameMetrics> result = new ArrayList<>();
while (true) {
  String s = reader.readLine();
  if (s == null) {
    break;  // end reached
  }
  if (s.trim().isEmpty()) {
    continue;  // Skip empty line
  }
  int cut = s.indexOf(':');
  if (cut == -1) {
    cut = s.indexOf('-');
    if (cut == -1) {
      continue;
    }
    metricsHolder = new HashMap<String, String>();
    current = new NameMetrics();
    current.setName(s.substring(0, cut));
    result.add(current);
  } else if (current != null) {
    metricsHolder.put(s.substring(0, cut), s.substring(cut + 1));
    current.setMetrics(metricsHolder);
  }
}

I'd like a code review to see whether this can be improved in any way?

Answer 1

Your current code is very good: it reads easily, it is properly indented and clean. I am sure one could come up with a complicated regular expression for parsing this String but using a StringReader like you have done is simple and effective. It is dynamic with regard to the number of elements to put in the map.

I only have a couple of comments:

• You are trimming each line only to determine if they are blank with

  if (s.trim().isEmpty()) {
      continue;  // Skip empty line
  }
  

  If you expect lines to have spaces to trim then you could do it all the time.

• Instead of having a setMetrics method that takes a map, I would have a addMetric method that adds a key/value pair into the map.

  public void addMetric(String key, String value) {
      metrics.put(key, value);
  }
  

  This way, you don't need to build the map yourself in the while loop.

• To prevent from NullPointerException, it is a good idea to always initiaze maps and lists when they are in instance variables. You can do it inline like the following:

  public class NameMetrics {
  
      private String name;
      private Map<String, String> metrics = new HashMap<>();
  
  } 
  

  or in the default constructor.

• The variable s in String s = reader.readLine(); could have a better name, like line.
• The while(true) loop with an explicit break works correctly, but it might be easier to follow with the traditional pattern while ((line = reader.readLine()) != null) {.

With those couples changes, you can have:

BufferedReader reader = new BufferedReader(new StringReader(response));
NameMetrics current = null;
List<NameMetrics> result = new ArrayList<>();
String line;
while ((line = reader.readLine()) != null) {
    line = line.trim();
    if (line.isEmpty()) {
        continue; // Skip empty line
    }
    int cut = line.indexOf(':');
    if (cut == -1) {
        cut = line.indexOf('-');
        if (cut == -1) {
            continue;
        }
        current = new NameMetrics();
        current.setName(line.substring(0, cut));
        result.add(current);
    } else if (current != null) {
        current.addMetric(line.substring(0, cut), line.substring(cut + 1));
    }
}

Answer 2

First, I'd like to point out that if are you storing this from your program and reading it back from a file later, you'd be better off Serializing it. But since you're being given this file, let's move on.

Infinite Loops

It's generally discouraged to use while(true) loops. It's not particularly bad (in fact it's sometimes better), but it's always best to look for an alternative for readability sake. Instead, let's use while ((line = reader.readLine()) != null).

Initializing variables as null

I also see you initialized a few variables as null, but that isn't necessary since it's implied for classes. (Also note primitive variables are implied as 0 during initialization).

Using the output format to your advantage

You also have a cut variable that gets the index of the tokens, and then skips the line if it's blank. However, we know that the output is always going to be grouped into three lines, and we always know what those lines are going to be. We also know the data you really want is going to be the first three characters for the name, and everything after the first five characters for the tokens. Let's use that and make everything clear!

Putting it all together

String response = restTemplate.getForObject(url, String.class);
BufferedReader reader = new BufferedReader(new StringReader(response));
NameMetrics current;
Map<String, String> metricsHolder;
List<NameMetrics> result = new ArrayList<>();

String name, load, peak;

String nameline;
while ((nameline = reader.readLine()) != null) {
    name = nameLine.substring(0, 3);
    load = reader.readLine().substring(5);
    peak = reader.readLine().substring(5);
    reader.readline(); //eat empty line

    current = new NameMetrics();
    metricsHolder = new HashMap<>();    //No need to repeat the stuff inside the diamond operator
    metricsHolder.put("Load", load);
    metricsHolder.put("Peak", peak);

    current.setName(name);
    current.setMetrics(metricsHolder);
    result.add(current);
}

Handling varying keys and names

When there are a variable number of keys, we still know that they're all generally in the same format of the name followed dashes, then a list of key:value lines, then a blank line. We can still use this to our advantage as follows:

String name;
String[] mapValues;

final int KEY = 0, VALUE = 1;
String nameline, keyLine;
while ((nameline = reader.readLine().trim()) != null) {    //Loop until the end of the file
    name = nameLine.substring(0, nameLine.indexOf("-") + 1); //Plus because substring excludes last index

    metricsHolder = new HashMap<>();    //No need to repeat the stuff inside the diamond operator
    while (!(keyLine = reader.readLine().trim()).isEmpty()) {   //Loop until block ends with an empty line
        mapValues = reader.readLine().split(":");   //Split key:value format
        metricsHolder.put(mapValues[KEY], mapValues[VALUE]);
    }

    current = new NameMetrics();
    current.setName(name);
    current.setMetrics(metricsHolder);
    result.add(current);
}