I would like to write more functional java code in my work. But how can I refactor something like bellow.

That code works, it has to to parse string like 100/100 and return pair of Integer.

private static final Pattern pattern = Pattern.compile("^(\\d )(/)(\\d )$");

public static Optional<Pair<Integer, Integer>> foo(String str) {
          var matcher = pattern.matcher(str != null ? str : "");
          return matcher.find() && matcher.groupCount() == 3 ?
            Optional.of(new Pair<Integer,Integer>(Integer.parseInt(matcher.group(1)),
              Integer.parseInt(matcher.group(3)))):
            Optional.empty();
        }
}

CodePudding user response：

public static Optional<Pair<Integer, Integer>> foo(String input) {
  return Optional.ofNullable(input)
    .map(str -> pattern.matcher(str))
    .filter(matcher -> matcher.find() && matcher.groupCount() == 3)
    .map(matcher -> new Pair<Integer, Integer>(
      Integer.parseInt(matcher.group(1)),
      Integer.parseInt(matcher.group(3))
    ));
}

It is best to always start with an Optional that way you avoid all null checks. UUsually Optional.of() or Optional.empty() should be avoided for only making a final result:

• Optional.of() without any further operations is a code smell. It means you never had to do anything that required null checks to begin with. Yes, there are such cases but they are rare. Always look closely if you find such code.

  Normally, code should start with Optional.of() or Optional.ofNullable() and continue with .map() or other methods on it.

• Optional.empty() is similar. It means you never needed to even check for nullity, just returned an empty result. Well, there are some cases you can take this shortcut but usually the point of Optional is to start with something and operate on it. The result of the operations can then produce an empty result.

  Typically correct usage of Optional.empty() is done in the beginning - some check is made first and an empty optional is produced as an early return. Be wary of returning Optional.empty() last or after you already have an Optional constructed.

CodePudding user response：

First, the pattern needs to be fixed to allow several digits and there's no need to use a separate group for / delimiter:

pattern = Pattern.compile("^(\\d )/(\\d )$");

Next, it may be better to use Optional.ofNullable for the input parameter, and Optional::flatMap to handle Optional<Pair> returned after parsing the input string.

public static Optional<Pair<Integer, Integer>> foo(String str) {
    return Optional.ofNullable(str)
        .flatMap(s -> pattern.matcher(s)
            .results()
            .map(mr -> new Pair<>(Integer.parseInt(mr.group(1)), Integer.parseInt(mr.group(2)))) // Stream<Pair>
            .findFirst() // Optional<Pair>
        );
}

Tests:

System.out.println(foo("100/100"));
System.out.println(foo(null));
System.out.println(foo("abcd"));

Output:

Optional[100, 100]
Optional.empty
Optional.empty

Corrected Java 8 friendly solution by VLAZ could look like this:

public static Optional<Pair<Integer, Integer>> foo(String str) {
  return Optional.ofNullable(str)
    .map(pattern::matcher)
    .filter(Matcher::matches) // no need to use find() for the given pattern
    .map(matcher -> new Pair<>(
      new Integer(matcher.group(1)),
      new Integer(matcher.group(2))
    ));
}