2
\$\begingroup\$

I'm trying to get more familiar with Java 8 streams as they seem to be very powerful (and shorter), so I rewrote a method to use streams. However I'm not very satisfied with it and would like some suggestions on how to make better use of streams. The code itself has multiple nested loops, so it's not very easy. Also I'm forced to use final arrays to write the variables inside the lambda loops.

// t1, t2 are List<Triangle> passed as parameter
Triangle original = t1.get(0);
t2.forEach(transformed -> 
{

    for (int[] permutation : permutations)
    {
        Matrix trial = calculateMatrix(original.getVertices(), applyPermutation(transformed.getVertices(), permutation));

        final double[] sum = {0};
        points1.stream().map(v -> v.extend(1)).map(trial::multiply).forEach(v -> sum[0] += points2.stream().mapToDouble(v2 -> distanceCheated(v, v2)).min().orElse(0));

        if (bestDeviation[0] == null || sum[0] < bestDeviation[0])
        {
            second[0] = transformed;
            bestDeviation[0] = sum[0];
            bestMatrix[0] = trial;
        }
    }
});

EDIT: So as I aparently left out many important information, i try to provide them here:

The first obvious missing information is the connection between points1/2 and t1/2. points1/2 are plain two lists of points contained in two images (original and transformed). t1/2 are lists of triangles built with delauney triangulation using the points from points1/2. The goal is to calculate the transformation matrix used, to create the second set of points.

I posted the whole class here: https://gist.github.com/anonymous/8f21597d8a51c8960de7 Vertex is used as a Vector (mathematical) Triangle consists of 3 Vertices

\$\endgroup\$
  • 1
    \$\begingroup\$ This code is missing quite a lot of surrounding context (and goal) to provide a really in-depth review of your code. I can surely give some stylistic suggestions, though. Unless you give more context I personally will refrain from making suggestions about changing your lambdas and / or judging the effectiveness of your approach. \$\endgroup\$ – Vogel612 Mar 17 '15 at 19:31
3
\$\begingroup\$

The way you have asked this question, the code is hard to review, as we have no idea what your goal is, or what points1 is, and whether it has anything to do with t1, etc.

I can say, though, that the use of a final double[] sum = {0} to work around the requirement that lambdas must work with final variables is bizarre. It's probably wrong, too, if parallel computing is involved. A reasonable way to sum a DoubleStream is to use DoubleStream.sum().

double sum = points1.stream()
                    .map(v -> v.extend(1))
                    .map(trial::multiply)
                    .mapToDouble(v ->
                         points2.stream()
                                .mapToDouble(v2 -> distanceCheated(v, v2))
                                .min().orElse(0))
                    .sum();

But I see that you seem to be using the same workaround with second, bestDeviation, and bestMatrix.

private static Scenario scenario(… points1, … points2, Triangle original, Triangle transformed, int[] permutation) {
    Scenario s = new Scenario();
    s.second = transformed;
    s.trial = calculateMatrix(original.getVertices(), applyPermutation(transformed.getVertices(), permutation));
    s.deviation = points1.stream()
                         .map(v -> v.extend(1))
                         .map(trial::multiply)
                         .mapToDouble(v -> …)
                         .sum();
    return s;
}

… your function …
    Triangle original = t1.get(0);
    Scenario best = t2.flatMap(transformed ->
                           Arrays.stream(permutations)
                                 .map(permutation -> scenario(points1, points2, original, transformed, permutation)))
                      .min((dev1, dev2) -> dev1.deviation - dev2.deviation)
                      .orElse(null);
\$\endgroup\$
  • \$\begingroup\$ That's a really nice solution. Thanks a lot \$\endgroup\$ – dnanon Mar 18 '15 at 10:00
3
\$\begingroup\$

Since (as mentioned in my comment) the question is very tightly scoped, this review will be mostly style-focused:

Names

Some of your names are good. Some of your names are bad. I didn't see anything really ugly though, so you got that going for you ;)

Some examples of the good names include:

original, permutation, bestDeviation, trial

These names are speaking for themselves, it's almost instantly clear what they mean.

On the other hand there's things like:

t1, t2, v, v2

These names are completely meaningless. I can't tell what they stand for, I have no idea (not in the slightest) what is in the variables with these names. It would tremendously help to understand your code's purpose, if I (or Mr. Maintainer) knew what the smallest units the code works with are.

I'd guesstimate you're operating on Vectors in a graphical representation of bodies, but I simply cannot be sure.

Anyways, from naming to:

Spacing and Linebreaks:

Thge code could use some additional linebreaks here and there, if only just to reduce the "verical complexity" of your code.

Normally I try to portion my code so you can read it one line at a time, and almost instantly understand what happens on it.

That's extremely hard when I need to scroll and there's multiple (low-level) transformations and operations. Try to write code, which doesn't take long to skim over and get the gist of.

points1.stream().map(v -> v.extend(1)).map(trial::multiply).forEach(v -> sum[0] += points2.stream().mapToDouble(v2 -> distanceCheated(v, v2)).min().orElse(0));

then becomes:

points1.stream().map(v -> v.extend(1))
        .map(trial::multiply)
        .forEach(v -> sum[0] += points2.stream()
                .mapToDouble(v2 -> distanceCheated(v, v2))
                .min().orElse(0)
        );

On the other hand there's one place where I feel you left too much space:

t2.forEach(transformed ->
{

    for (int[] permutation : permutations) {

I'd write it like this:

t2.forEach(transformed -> {
    for (int[] permutation : permutations) {

Other than this, there's no stylistic points to make here, and as mentioned I don't want to review the functionality without more information and context.

\$\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.