current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

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

share|improve this question
1  
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. –  Vogel612 Mar 17 at 19:31

2 Answers 2

up vote 3 down vote accepted

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);
share|improve this answer
    
That's a really nice solution. Thanks a lot –  dnano91 Mar 18 at 10:00
up vote 3 down vote

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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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