Skip to main content
Commonmark migration
Source Link

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

##Names

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:

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.

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.

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.

Source Link
Vogel612
  • 25.5k
  • 7
  • 59
  • 141

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.