Parsing complex string data

Question

I have a complex strings like the following:

$company=>Company(code->MyCompany)
$brand=>Brand(code->Brand_1, company->$company)

and then I prepare objects out of them as follows:

Macro [key=$company, type=Company, map={code=MyCompany}]
Macro [key=$brand, type=Brand, map={ company=$company, code=Brand_1}]

using the following code for parsing. Is there a better way?

public class Macro {

    private String key;

    private String type;

    private Map<String, String> map;
}

public static Macro getMacro(String str) {
    final Macro macro = new Macro();
    final Map<String, String> map = new HashMap<String, String>();
    final Matcher matcher = Pattern.compile("\\(([^)]+)\\)").matcher(str);
    while (matcher.find()) {
        final List<String> list = Arrays.asList(matcher.group(1).split(",")).stream()
                .collect(Collectors.toList());
        list.stream().forEach(a ->
        {
            final String[] ar = a.split("->");
            map.put(ar[0], ar[1]);
        });
    }
    str = str.replaceAll("\\(.*\\)", "");
    final String[] s = str.split("=>");
    macro.setMap(map);
    macro.setKey(s[0]);
    macro.setType(s[1]);
    return macro;
}

Answer 1

Pattern.compile("\\(([^)]+)\\)");

The Pattern is always the same so you may just as well make it a private static final constant. This will avoid having to recompile the pattern each time.

---

Arrays.asList(/*..*/).stream().collect(Collectors.toList());

Why this dance? Arrays.asList returns a List. You only do a stream on the return result.

    final List<String> list = Arrays.asList(matcher.group(1).split(","));
    list.stream().forEach(a ->
    {
        final String[] ar = a.split("->");
        map.put(ar[0], ar[1]);
    });

However a Pattern an return a Stream split according to the pattern directly:

Pattern COMMA_PATTERN = Pattern.compile("\\s*,\\s*");


COMMA_PATTERN.splitAsStream(matcher.group(3)).forEach(a ->
{
    //...
}

---

You can use a single match of a Pattern to find both the leading values and the string inside the parenthesis:

private static final Pattern FULL_PATTERN = Pattern.compile("^(\\$\\w+)\\s*=>\\s*(\\w+)\\s*\\(([^)]+)\\)$");
private static final Pattern KEY_VALUE_PATTERN = Pattern.compile("(\\w+)\\s*->\\s*([\\$\\w]+)");
private static final Pattern COMMA_PATTERN = Pattern.compile("\\s*,\\s*");

public static Macro getMacro(String str) {
    final Macro macro = new Macro();
    final Map<String, String> map = new HashMap<String, String>();
    final Matcher matcher = FULL_PATTERN.matcher(str);
    if(!matcher.matches()) {
        throw new IllegalArgumentException("...");
    }

    COMMA_PATTERN.splitAsStream(matcher.group(3)).forEach(a ->
    {
        final Matcher kvMatcher = KEY_VALUE_PATTERN.matcher(a);
        if(!kvMatcher.matches()){
             throw new IllegalArgumentException("...");
        }

        map.put(kvMatcher.group(1), kvMatcher.group(2));
    });
    macro.setMap(map);
    macro.setKey(matcher.group(1));
    macro.setType(matcher.group(2));
    return macro;

}

You'll note I added \\s* to various points in each pattern, This lets you ignore the whitespace that may surround the operators.

The \\w in a Pattern means a word character. If you only want alphanumeric +undescore then you'llneed to replace each instance of it with [\\p{Alnum}_]

Answer 2

Simplifications:

• Arrays.asList(...).stream() can be shorter (and faster) as Arrays.stream(...)

• In the same vein, the code collects the stream into a list, just to restream the list. Additionally using an Action (as in forEach) to collect results is semantically questionable. The whole while-loop should be written like this:

  while (matcher.find()) {
      map = Arrays.stream(matcher.group(1).split(","))
          .map(content -> content.split("->")
          .collect(Collectors.toMap(a -> a[0], a -> a[1]));
  }
  

• Since the Pattern instance you have does not change, I'd extract it into a static variable

• Instead of making this an external static method, I'd put it into the Macro class. This enables calling it as follows: Macro.fromString(...).

Last but not least this is a pretty nicely usable place for regex as follows:

static final Pattern macroPattern = 
    Pattern.compile("(\\$[^=]+)=>([A-Z][^\\(]+)\\((([^-]+)->([^\\)]+)(?:\\s*,\\s*)?\\)");

Now if java had named capture groups this would suddenly become much easier (~hint, hint, nudge nudge). How to use this pattern is something left as exercise to the reader

Answer 3

You are calling Stream.forEach(Consumer) with a Consumer whose action accesses shared state (namely the local and final variable map) without taking care of synchronization (a HashMap is not thread-safe). This is bad. Stream.forEach(Consumer) might look like a simple for-each loop, but it is more than that, because unlike the latter, it might process multiple stream elements simultaneously in multiple threads, so accessing shared state without synchronization is dangerous. A safer approach would have been to use forEachOrdered(Consumer) instead of forEach(Consumer).

The solution proposed by Vogel612 for this problem is even more elegant, but, technically, it is not equivalent to your procedure, because your procedure doesn't create a new Map for every iteration of while (matcher.find()), as opposed to Vogel612's code, which assigns a new Map to map for every loop iteration. However, the while (matcher.find()) loop is probably a design flaw anyway, because as far as I understand your question, the input string is not supposed to contain multiple parentheses-enclosed "maps" (not to be confused with key-value-"mappings"). This can be rectified by using a single regular expression for the whole input string, as the other answers have already suggested.