Converting to functional Java style

Question

How would one re-write the following in proper Java 8 functional style using filter, collector, etc:

private BigInteger calculateProduct(char[] letters) {

    int OFFSET = 65;

    BigInteger[] bigPrimes = Arrays.stream(
            new int[] { 2, 3, 5, 7, 11, 13, 17, 19, 23, 29, 31,
            37, 41, 43, 47, 53, 59, 61, 67, 71, 73, 79, 83, 89,
            97, 101, 103, 107, 109, 113 })
            .mapToObj(BigInteger::valueOf)
            .toArray(BigInteger[]::new);


    BigInteger result = BigInteger.ONE;
    for (char c : letters) {
        //System.out.println(c+"="+(int)c);
        if (c < OFFSET) {
            return new BigInteger("-1");
        }
        int pos = c - OFFSET;
        result = result.multiply(bigPrimes[pos]);
    }
    return result;
}


@Test public void test() {
    assertThat(calculateProduct(capitalize("carthorse"))).isEqualTo(calculateProduct(capitalize("orchestra")));

}


private char[] capitalize(String word) {
    return word.toUpperCase().toCharArray();
}

I want to re-write calculateProduct() method in a fluent Java-8 style, replacing the loop+assignments with more functional accumulators, collectors, etc. — Simeon Leyzerzon, Oct 01 '17 at 21:42

Eugene · Answer 1 · 2017-10-04T12:02:13.020

I can't tell why you want that, but may be this (which creates many more objects and is more verbose):

private static BigInteger calculateProduct(char[] letters) {

    int OFFSET = 65;

    BigInteger[] bigPrimes = Arrays.stream(
            new int[] { 2, 3, 5, 7, 11, 13, 17, 19, 23, 29, 31,
                    37, 41, 43, 47, 53, 59, 61, 67, 71, 73, 79, 83, 89,
                    97, 101, 103, 107, 109, 113 })
            .mapToObj(BigInteger::valueOf)
            .toArray(BigInteger[]::new);

    Optional<Character> one = IntStream.range(0, letters.length)
            .mapToObj(x -> letters[x])
            .filter(x -> x < OFFSET)
            .findAny();

    if (one.isPresent()) {
        return new BigInteger("-1");
    } else {
        return IntStream.range(0, letters.length)
                .mapToObj(x -> letters[x])
                .parallel()
                .reduce(
                        BigInteger.ONE,
                        (x, y) -> {
                            int pos = y - OFFSET;
                            return x.multiply(bigPrimes[pos]);
                        },
                        BigInteger::multiply);
    }

}

Why are you multiplying `BigInteger.ONE` to the result in every evaluation? Multiplying something with one doesn’t change the result, that applies `BigInteger` as well (and the OP’s code doesn’t do that). And generally, …why so complicated? There is no need to handle `char` values as `Character`, *especially* as there’s only arithmetic done on them. — Holger, Oct 04 '17 at 11:50

score 2 · Accepted Answer · answered Oct 04 '17 at 11:47

You may do it like this:

static final BigInteger[] PRIMES
    = IntStream.of(
        2, 3, 5, 7, 11, 13, 17, 19, 23, 29, 31, 37, 41, 43, 47, 53,
        59, 61, 67, 71, 73, 79, 83, 89, 97, 101, 103, 107, 109, 113)
    .mapToObj(BigInteger::valueOf)
    .toArray(BigInteger[]::new);

private BigInteger calculateProduct(char[] letters) {
    final int OFFSET = 65;
    final CharBuffer cb = CharBuffer.wrap(letters);
    if(cb.chars().anyMatch(c -> c<OFFSET))
        return BigInteger.ONE.negate();
    return cb.chars()
        .mapToObj(c -> PRIMES[c-OFFSET])
        .reduce(BigInteger.ONE, BigInteger::multiply);
}

Note that moving the creation of the PRIMES array out of the method, to avoid generating it again for each invocation, is an improvement that works independently of whether you use loops or “functional style” operations.

Also, your code doesn’t handle characters being too large, so you might improve the method to

private BigInteger calculateProduct(char[] letters) {
    final int OFFSET = 65;
    final CharBuffer cb = CharBuffer.wrap(letters);
    if(cb.chars().mapToObj(c -> c-OFFSET).anyMatch(c -> c<0||c>PRIMES.length))
        return BigInteger.ONE.negate();
    return cb.chars()
        .mapToObj(c -> PRIMES[c-OFFSET])
        .reduce(BigInteger.ONE, BigInteger::multiply);
}

I guess the parameter array `char[] letters` could also be made `final`, right? — Simeon Leyzerzon, Feb 06 '18 at 02:13
Yes, you could declare `letters` as `final` too. But that’s just a stylistic choice, without any impact on the code (it’s already *effectively final*). The only variable where it has an impact, is the `OFFSET` variable, which is a compile-time constant when it has the `final` modifier. — Holger, Feb 06 '18 at 09:21

Converting to functional Java style

2 Answers2