Using a wrapper on a primitive as a generic for an interface used for Java lambda

Question

I apologize for that title, lol.

I have a Java method that I'm writing where I want to be able to pass in an array of Objects and two interfaces that will be used for lambda expressions that specify a particular value to use for calculation.

It's part of a larger class that I want to use for all kinds of Statistics calculations, but I started with calculating correlation because it's most relevant to the specific problem I want to solve.

import java.lang.IllegalArgumentException;
import java.util.Arrays;
import java.util.stream.Stream;
import java.util.stream.Collectors;
import java.util.List;

public class Statistics
{
    //1. This method is really unreadable
    public static <O> double getCorrelation(O[] a, Fetchable<O, Double> dataPointA, Fetchable<O, Double> dataPointB)
    {
        Double[] temp = Arrays.stream(a).map(x -> dataPointA.fetch(x)).collect(Collectors.toList()).toArray(new Double[0]);
        Double[] temp2 = Arrays.stream(a).map(x -> dataPointB.fetch(x)).collect(Collectors.toList()).toArray(new Double[0]);
        return getCorrelation(temp, temp2);
    }

    public static double getCorrelation(double[] a, double[] b)
    {
        if (a.length != b.length)
        {
            //2. Is this the best Exception for this case?
            throw new IllegalArgumentException();
        }
        double sumA = 0.0;
        double sumB = 0.0;
        double sumSquareA = 0.0;
        double sumSquareB = 0.0;
        double sumAB = 0.0;
        for(int i = 0; i < a.length; i++)
        {
            sumA += a[i];
            sumB += b[i];
            sumSquareA += a[i] * a[i];
            sumSquareB += b[i] * b[i];
            sumAB += a[i] * b[i];
        }
        int n = a.length;
        return (n * sumAB - sumA * sumB) / (Math.sqrt(n * sumSquareA - sumA * sumA) * Math.sqrt(n * sumSquareB - sumB * sumB));
    }

    private static double getCorrelation(Double[] a, Double[] b)
    {
        if (a.length != b.length)
        {
            //2. Is this the best Exception for this case?
            throw new IllegalArgumentException();
        }
        //3. Is there a better way to do this conversion from Double[] to double[]?
        double doubleArrA[] = new double[a.length];
        double doubleArrB[] = new double[a.length];
        for (int i = 0; i < a.length; i++)
        {
            doubleArrA[i] = (double)a[i];
            doubleArrB[i] = (double)b[i];
        }
        return getCorrelation(doubleArrA, doubleArrB);
    }

    interface Fetchable<T1, T2>
    {
        public T2 fetch(T1 a);
    }

    //main function for testing
    public static void main(String[] args)
    {
        //this class is defined in another file; it's a simple class with four public doubles I made just to test
        TestDataPoint a[] = new TestDataPoint[5];
        a[0] = new TestDataPoint();
        a[0].w = 3;
        a[0].x = 0;
        a[0].y = 4;
        a[0].z = 9;
        a[1] = new TestDataPoint();
        a[1].w = 1;
        a[1].x = 8;
        a[1].y = 3;
        a[1].z = 2;
        a[2] = new TestDataPoint();
        a[2].w = 7;
        a[2].x = 4;
        a[2].y = 4;
        a[2].z = 0;
        a[3] = new TestDataPoint();
        a[3].w = 3;
        a[3].x = 1;
        a[3].y = 0;
        a[3].z = 1;
        a[4] = new TestDataPoint();
        a[4].w = 6;
        a[4].x = 3;
        a[4].y = 9;
        a[4].z = 8;
        a[1] = new TestDataPoint();
        System.out.println(getCorrelation(a, p -> p.w, q -> q.z));
        System.out.println(getCorrelation(a, p -> p.x, q -> q.z));
        System.out.println(getCorrelation(a, p -> p.y, q -> q.z));
        System.out.println(getCorrelation(a, p -> p.z, q -> q.z));
    }
}

It works pretty well; I've tried it and it seems to do exactly what I want. There are a few things I want to look at, though:

That crazy method is absurdly unreadable.
Is IllegalArgumentException the best Exception for the case where the method cannot execute properly because arrays of differing lengths are provided?
Is there any easily readable way to convert Double[] to double[] without the loop I used in getCorrelation(Double[] a, Double[] b)?

Thanks in advance.

Michael · Accepted Answer · 2018-07-02 15:53:55Z

To focus on your initial questions

1) That crazy method is absurdly unreadable.

I don't think it's too bad. You can improve it by using method references rather than lambdas and the toArray method of Stream:

Double[] temp = Arrays.stream(a).map(dataPointA::fetch).toArray(Double[]::new);
Double[] temp2 = Arrays.stream(a).map(dataPointB::fetch).toArray(Double[]::new);
return getCorrelation(temp, temp2);

2) Is IllegalArgumentException the best Exception for [this] case

Yeah, it's fine. "Best" is subjective. Honestly, it doesn't really matter. I would, however, provide a more detailed message which explains why the arguments were invalid:

throw new IllegalArgumentException("Input arrays must be the same length");

3) Is there any easily readable way to convert Double[] to double[] without the loop?

Yes, it's a bit better if you use streams:

final double[] doubleArrA = Stream.of(a).mapToDouble(x -> x).toArray();
final double[] doubleArrB = Stream.of(b).mapToDouble(x -> x).toArray();

Some other thoughts

Declare variables as final if you don't intend to change them. It reduces the chance of accidental errors on your part, and helps readers of your code forget about the possibility that the variable might change.
Using meaningful identifiers. a, b, w, x, y, z, temp1, temp2: these could all be improved. Single character identifiers are only really okay in a few specific sitations, such as when using them as loop counters, or generic type parameters.
At the cost of iterating over the arrays multiple times (don't worry about the performance until you know it's a problem), your getCorrelation method could initialise most of the sums in the same line as they're declared, thus allowing them to be declared as final:

final double sumA = DoubleStream.of(a).sum();
final double sumB = DoubleStream.of(b).sum();
final double sumSquareA = DoubleStream.of(a).map(x -> x * x).sum();
final double sumSquareB = DoubleStream.of(b).map(x -> x * x).sum();

double sumAB = 0.0;
for(int i = 0; i < a.length; i++)
{
    sumAB += a[i] * b[i];
}

Use comments to document unusual formulas or ideas. (n * sumAB - sumA * sumB) ... is hard to understand at a glance. It might benefit from a comment identifying the mathematical proof (e.g. //Pythagorean theorem) or even a link to the Wikipedia page which explains it
Do not use public access as your default. Start by declaring everything as private and gradually increase the visibility as necessary.
Define your own constructors. TestDataPoint should have a constructor which takes 4 arguments. You should not rely on initialising the properties one at a time because this makes it possible to leave yourself with half-initialised objects.

class TestDataPoint {
    //...

    TestDataPoint(double w, double x, double y, double z) {
        this.w = w;
        this.x = x;
        this.y = y;
        this.z = z;
    }
}

I suggest make attribute of TestDataPoint private and final — Peter1982
– Peter1982, Commented Jul 2, 2018 at 16:30
Thanks, there's a lot of great stuff in here. But I wonder why "Arrays.stream(a).map(dataPointA::fetch).toArray(Double[]::new)" works but "Arrays.stream(a).map(dataPointA::fetch).toArray(new Double[0])" doesn't. — Devsman
– Devsman, Commented Jul 2, 2018 at 17:01
@Devsman This is because (as opposed to Collection.toArray) the stream does not accept a ready-made array with the option to discard and resize, but an IntFunction<A[]> to generate the array. Double[]::new is such an IntFunction, whereas new Double[0] is just a simple object. — mtj
– mtj, Commented Jul 3, 2018 at 6:20

Stack Exchange Network

Using a wrapper on a primitive as a generic for an interface used for Java lambda

1 Answer 1

You must log in to answer this question.

Hot Network Questions

Using a wrapper on a primitive as a generic for an interface used for Java lambda

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions