18
\$\begingroup\$

I am experimenting with streams and lambdas in Java 8. This is my first serious foray using functional programming concepts; I'd like a critique on this code.

This code finds the Cartesian product of three sets of ints. My questions/complaints about this snippet are:

  • Are nested flatMaps really the best (most understandable, most CPU/memory efficient) way to do an arbitrary Cartesian product? Or should I stick to imperative style for Cartesian products?
  • Is there a better way to format nested lambdas (see, e.g., my use of flatMap)?

I just learned about Collectors; those might be what I need. I'm still learning how to best combine these things.


long count = IntStream.rangeClosed(0,9) /* 0 <= A <= 9 */
    .parallel()
    .mapToObj(Integer::valueOf)
    .flatMap(a -> 
            IntStream.rangeClosed(0,9) /* 0 <= B <= 9 */
            .mapToObj(Integer::valueOf)
            .flatMap(b ->
                    IntStream.rangeClosed(0,9) /* 0 <= C <= 9 */
                    .mapToObj(c -> (new Product()).A(a).B(b).C(c))
            )
     ).count();
System.out.println("Enjoy your Cartesian product." + count);

UPDATE: I deliberately omitted boilerplate code and class/method definitions, for brevity. The only important thing about Product is that it stores three ints.

\$\endgroup\$
  • 1
    \$\begingroup\$ What is Product... you should probably include that in the review? \$\endgroup\$ – rolfl Mar 22 '14 at 0:39
  • 1
    \$\begingroup\$ I took the liberty of rolling-back the edit you made because it invalidated parts of the answers that were previously given. Have a look at the meta post about this exact thing \$\endgroup\$ – rolfl Mar 22 '14 at 15:40
15
\$\begingroup\$

Your code is odd in the sense that it is going to a lot of effort to calculate that 103 is 1000.

I understand why you are doing it, but I took the liberty of changing the count() terminating function and replacing it with:

StringBuilder sb = new StringBuilder();

.......
.forEachOrdered(prod -> sb.append(prod.toString()).append("\n");

This way we can store each result, instead of just counting it.

There are a few things to go through here.

Product

This class looks awkward:

 .mapToObj(c -> (new Product()).A(a).B(b).C(c))

Why not just have a useful constructor on Product like:

    public Product(int a, int b, int c) {
        super();
        this.a = a;
        this.b = b;
        this.c = c;
    }

Then, your mapping would look like:

 .mapToObj(c -> new Product(a, b, c))

Magic Numbers

You have 0 and 9 as magic numbers in multiple places. These should be declared as (effective) constants, or calculated somehow. With the 9 value especially, if you want to change the size of the product you have to change it in three places.

int vs. Integer

You start off with an IntStream, but then convert it to a Stream<Integer>. Why? If you can leave things as primitives, you should.

The actual stream

The most concerning aspect is the stream itself.

The flatMap operation is not doing what I think you think it does....

Let's consider the contents of the stream as it goes through things:

  1. 'a' range from 0-9 IntStream
  2. convert each int to an Integer
  3. for each Integer, flatMap that Integer
  4. the flatMap creates an IntStream which it converts to a Stream
  5. even though we are flatMapping the 'a' value, we are not using the 'a' value in the mapping. All we are really doing is creating a new 'b' value, and who cares that the stream now has the b values streaming through....
  6. with these 'b' values on the stream, we then flatMap again....
  7. again, we ignore what the actual 'b' value is, abut we generate a new 'c' value in a third nested stream, and then we combine the 'a' and 'b' values which are 'in scope', with the 'c' value from the stream, and we make a Product.

The point of what I am saying is that you actually are only using the Stream in the most inside IntStream, the 'c' stream. All the other streams are just 'tokens' that you need to count the events you are doing. A for-loop is the right thing for this construct (if it was not for the parallelism). If you include the parallelism, then a fork-join process is right. Not a stream.

My Attempt

I am not claiming this is right, or best practice, but consider this attempt to get essentially the same result as you:

/** Append an int to an array after the last array item **/
public static final int[] append(int[] base, int val) {
    int[] ret = Arrays.copyOf(base, base.length + 1);
    ret[base.length] = val;
    return ret;
}

/** Stream an int range and map to an appended array */
private static final Stream<int[]> appendInts(int[] base, int size) {
    return IntStream.rangeClosed(0, size).mapToObj(a -> Cartesian.append(base, a));
}

public static void main(String[] args) {

    final int last = 9;
    final StringBuilder sb = new StringBuilder();
    IntStream.rangeClosed(0,last) /* 0 <= A <= 9 */
             .parallel()
             .mapToObj(a  -> new int[]{a})
             // stream is int[1] array
             .flatMap(ab  -> appendInts(ab, last))
             // stream is int[2] array
             .flatMap(abc -> appendInts(abc, last))
             // stream is int[3] array
             .forEachOrdered(res -> sb.append(Arrays.toString(res)).append("\n"));;
    System.out.println("Enjoy your Cartesian product." + sb.toString());
}

Update

I have rewritten the OP's code to extract the stream preparations. Consider the following code:

    // convert two input values to a stream consisting of an arrays of int
    BiFunction<Integer, Integer, Stream<int[]>> stage3 = (a,b) -> {
        return IntStream.rangeClosed(0, last).mapToObj(c -> new int[]{a, b, c});
    };

    // convert an input value to a stream consisting of an arrays of int
    Function<Integer, Stream<int[]>> stage2 = a -> {
        Stream<Integer> s = IntStream.rangeClosed(0, last).boxed();
        Stream<int[]> ret = s.flatMap(b -> stage3.apply(a, b));
        return ret;
    };

    IntStream.rangeClosed(0,last) /* 0 <= A <= 9 */
            .parallel()
            .boxed()
            .flatMap(a -> stage2.apply(a))
            .map(r -> Arrays.toString(r))
            .forEachOrdered(System.out::println);

Which is essentially what the OP's code does.

\$\endgroup\$
11
\$\begingroup\$

I do not have much to add to @rofl's answer, but I do have another way to show to create Cartesian Products while using streams, which I think might be very interesting aswell.

Important note

I have made a mistake while reading the intention of the OP's post. I have confused Cartesian Products with calculating products of the elements of Cartesian Products. I will leave my old answer here as the explanation still holds, and under this answer, I will post the new version of the code, which does do what is expected.


Old answer

I will also assume that you are collecting the results into a List<Integer>, but you can do anything you want with them of course. Note that Collectors.toList() ensures that the data is ordered, so it is equivalent to printing out in order with forEachOrdered(System.out::println), however in the collector it creates a list obviously.

First verbose version:

private void printCartesianProductsVerbose() {
    int min = 0;
    int max = 9;
    Supplier<IntStream> streamSupplier = () -> IntStream.rangeClosed(min, max);
    IntBinaryOperator productOperator = (a, b) -> a * b;
    List<Integer> list = streamSupplier.get()
            .flatMap(a -> streamSupplier.get().map(b -> productOperator.applyAsInt(a, b)))
            .flatMap(a -> streamSupplier.get().map(b -> productOperator.applyAsInt(a, b)))
            //Do whatever you want below here
            .boxed()
            .collect(Collectors.toList());
    list.forEach(System.out::println);
}

I will explain what I am doing here, and you should see most of the points as tips to produce better code:

  1. Get rid of the magic numbers, define min and max clearly. Later this might become method input, so you can pass it directly to the rangeClosed() method.
  2. Create a Supplier<IntStream> for your IntStream.rangeClosed(min, max), which you would else repeat the whole time. Less chance of mistakes while copying code now as a plus, and easier to change.
  3. Create a IntBinaryOperator to encapsulate the product operator that happens when calculating Cartesian Products.
  4. Turn the IntStream into a List<Integer>.

    4.1. First you need to declare that you want to end up with a list.

    4.2. Then you call streamSupplier.get() to get your stream.

    4.3. Now the interesting part happens, I flat map the a current integer of the stream, to a new IntStream, note that here I still have access to the a integer.

    4.4. Then I apply a mapping to the newly obtained IntStream, and map the b integer to the product of a and b.

    4.5. Now the newly obtained IntStream contains the Cartesian Products, and these are via the flatMap() brought back into the original stream.

    4.6. Repeat the same flatMap for the Cartesian Product of three integers.

  5. Box the ints to Integers, since they are boxed in List<Integer> anyway and it will be easier to write a Collector for a Stream<Integer> than for primitive streams.

  6. Collect them into a List<Integer> via .collect(Collectors.toList()).
  7. Print the contents of the list.

Generalized version:

The concensus in this version is to generalize everyting as much as possible, such that different functionality can be plugged in at later times. What if you want to make Cartesian Sums, if that even is a thing?

private void printCartesianProducts() {
    int min = 0;
    int max = 9;
    Supplier<IntStream> streamSupplier = () -> IntStream.rangeClosed(min, max).parallel();
    IntBinaryOperator productOperator = (a, b) -> a * b;
    List<Integer> list = createCartesianStream(streamSupplier, productOperator, 3)
            //Do whatever you want below here
            .boxed()
            .collect(Collectors.toList());
    list.forEach(System.out::println);
}

private IntStream createCartesianStream(final Supplier<IntStream> supplier, final IntBinaryOperator binaryOperator, final int count) {
    Objects.requireNonNull(supplier);
    Objects.requireNonNull(binaryOperator);
    if (count < 1) {
        throw new IllegalArgumentException("count < 1: count = " + count);
    }
    IntStream intStream = supplier.get();
    for (int i = 1; i < count; i++) {
        intStream = intStream.flatMap(a -> supplier.get().map(b -> binaryOperator.applyAsInt(a, b)));
    }
    return intStream;
}

This code executes your flatMap a number of times (The first flatMap is essentially one of 1 to the method used while flat mapping), with every functionality pluggable.

Styling lambdas

Lastly some notes about styling the lambdas, I prefer if every chaining operation is on one line such that it is clearly what the code does. It gets a bit trickier inside flat maps, but even there I prefer keeping it one line as long as it does not get too long.


New answer

The change to the newer answer is not so big, what I do now is, instead of calculating the answer, I will map the elements to a List<Integer>.

Verbose version:

private void printCartesianProductsVerbose() {
    int min = 0;
    int max = 9;
    Supplier<Stream<List<Integer>>> streamSupplier = () -> IntStream.rangeClosed(min, max)
            .mapToObj(this::createListAndAdd)
            .parallel();
    BinaryOperator<List<Integer>> addOperator = this::addToList;
    List<List<Integer>> list = streamSupplier.get()
            .flatMap(a -> streamSupplier.get().map(b -> addOperator.apply(a, b)))
            .flatMap(a -> streamSupplier.get().map(b -> addOperator.apply(a, b)))
            //Do whatever you want below here
            .collect(Collectors.toList());
    list.forEach(System.out::println);
}

private <E> List<E> createListAndAdd(final E element) {
    List<E> list = new ArrayList<>();
    list.add(element);
    return list;
}

private <E> List<E> addToList(final List<E> in1, final List<E> in2) {
    List<E> list = new ArrayList<>(in1.size() + in2.size());
    list.addAll(in1);
    list.addAll(in2);
    return list;
}

One notable design choice I made here is to use the type parameter <E> in the methods, instead of the concrete Integer type, such that when changing the type on a later point, I don't need to rewerite those methods.

Generalized version

private void printCartesianProducts() {
    int min = 0;
    int max = 9;
    Supplier<Stream<List<Integer>>> streamSupplier = () -> IntStream.rangeClosed(min, max)
            .mapToObj(this::createListAndAdd)
            .parallel();
    BinaryOperator<List<Integer>> addOperator = this::addToList;
    List<List<Integer>> list = createCartesianStream(streamSupplier, addOperator, 3)
            //Do whatever you want below here
            .collect(Collectors.toList());
    list.forEach(System.out::println);
}

private <E> Stream<List<E>> createCartesianStream(final Supplier<Stream<List<E>>> supplier, final BinaryOperator<List<E>> binaryOperator, final int count) {
    Objects.requireNonNull(supplier);
    Objects.requireNonNull(binaryOperator);
    if (count < 1) {
        throw new IllegalArgumentException("count < 1: count = " + count);
    }
    Stream<List<E>> stream = supplier.get();
    for (int i = 1; i < count; i++) {
        stream = stream.flatMap(a -> supplier.get().map(b -> binaryOperator.apply(a, b)));
    }
    return stream;
}

I believe no more comments need to be added, as the code still does the same as the old answer, but now it does the correct thing.

\$\endgroup\$
7
\$\begingroup\$

It seems to me that when you asked:

Or should I stick to imperative style for Cartesian products?

...it was an excellent question--and one that nobody's really tried to answer. What you seem to want to accomplish is apparently roughly equivalent to something like this1:

for (int i=0; i<10; i++)
    for (int j=0; j<10; j++)
        for (int k=0; k<10; k++)
            count++;
            // though presumably you'd really want something more like:
            // some_collection.Add(Product(i, j, k));

[With the commented out code assuming you add a constructor to Product like @rolfl suggested.]

This is short and simple. Pretty much anybody who knows any 'curly brace' style of language (or nearly anything else with some sort of for loop or a reasonable analog like Fortran's DO loop) can understand it very quickly and easily.

Assuming you're talking about real, production code here (not just exploring new features in Java) I'd have serious reservations about replacing code like this with the code from your question or the code in any of the other answers I've seen.

It's easy to get caught up in cool new features, trying to use them everywhere, whether they really make much sense or not. I don't want to be a wet blanket, ruining the fun of putting new features to use (because believe me, I've been there, and realize it can be a lot of fun) but for real, production code? I'd hate to be the guy who had to explain to his boss what he had gained from my replacing 4 lines of simple code that just about anybody can understand almost without even trying, with 18-20 lines of code that almost nobody can understand without rereading it at least a couple of times--and even at best, they need to know the latest version of Java pretty well to understand it even that quickly.

Bottom line: this doesn't seem to me like a particularly good use of the features. IMO, you're losing more than you gain.


1. Yes, you're also at least potentially running some of the code in parallel--but for such a trivial computation as this, chances are pretty good that parallel computation gains little or nothing. It'll almost certainly take longer to package the computation up as parallizable tasks, dispatch it to threads, then collect the results, than to just do the computation in a single thread.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.