Skip to main content
Code Review

Return to Answer

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.

lang-java

AltStyle によって変換されたページ (->オリジナル) /