7
\$\begingroup\$

I am just looking for code review and overall critique. I want to know what I can do better and how can I do things more efficiently in Java.

I need particular feedback on the use of my ArrayList in my colsToRows method and then how I converted it back to regular arrays in Java. What better step could I have taken there?

/*
double[][] studentScores ={{90,85,75,85,95},
{85,60,65,95,65},
{75,70,80,85,90},
{95,70,75,80,100},
{75,45,80,60,70}};
String[] studentNames = {"Jakob Baloski",
"Lucinda Pavlov",
"Daniele Hanston",
"Yusef Goldstein",
"Leona Rhee"
};
Your app is going to generate the following output to the console:
Jakob Baloski
Highest Score = 95.0
Lowest Score = 75.0
Mean = 86.0 Grade:B
Mean (lowest dropped) = 88.75
-------------------------------------
Lucinda Pavlov
Highest Score = 95.0
Lowest Score = 60.0
Mean = 74.0 Grade:C
Mean (lowest dropped) = 77.5
-------------------------------------
Daniele Hanston
Highest Score = 90.0
Lowest Score = 70.0
Mean = 80.0 Grade:B
Mean (lowest dropped) = 82.5
-------------------------------------
Yusef Goldstein
Highest Score = 100.0
Lowest Score = 70.0
Mean = 84.0 Grade:B
Mean (lowest dropped) = 87.5
-------------------------------------
Leona Rhee
Highest Score = 80.0
Lowest Score = 45.0
Mean = 66.0 Grade:D
Mean (lowest dropped) = 71.25
-------------------------------------
++++++++++++++++++++++++++++++++++++++++++++
Assignment  1
Highest Score = 95.0
Lowest Score = 75.0
Mean = 84.0 Grade:B
Mean (lowest dropped) = 86.25
-------------------------------------
Assignment  2
Highest Score = 85.0
Lowest Score = 45.0
Mean = 66.0 Grade:D
Mean (lowest dropped) = 71.25
-------------------------------------
Assignment 3
Highest Score = 80.0
Lowest Score = 65.0
Mean = 75.0 Grade:C
Mean (lowest dropped) = 77.5
-------------------------------------
Assignment 4
Highest Score = 95.0
Lowest Score = 60.0
Mean = 81.0 Grade:B
Mean (lowest dropped) = 86.25
-------------------------------------
Assignment 5
Highest Score = 100.0
Lowest Score = 65.0
Mean = 84.0 Grade:B
Mean (lowest dropped) = 88.75
-------------------------------------
 */



import java.util.ArrayList;
import java.util.List;

public class Main {

    public static void main(String[] args) {
        double[][] studentScores = {
                {90, 85, 75, 85, 95},
                {85, 60, 65, 95, 65},
                {75, 70, 80, 85, 90},
                {95, 70, 75, 80, 100},
                {75, 45, 80, 60, 70}
        };

        String[] studentNames = {
                "Jakob Baloski",
                "Lucinda Pavlov",
                "Daniele Hanston",
                "Yusef Goldstein",
                "Leona Rhee"
        };
        List<List<Double>> assignmentScores = colsToRows(studentScores);

        // PRINT STUDENT RESULTS
        int i = 0;
        for(double[] score : studentScores) {
            System.out.println(studentNames[i]);
            System.out.println("Highest score = " + max(score));
            System.out.println("Lowest score = " + min(score));
            System.out.println("Mean = " + mean(score) + " Grade:" + gradeLetter(mean(score)));
            System.out.println("Mean (lowest dropped) = " + meanLowDrop(score));
            System.out.println("-------------------------------------");
            i++;
        }
        System.out.println();
        System.out.println();
        System.out.println("+++++++++++++++++++++++++++++++++++++++++++++++");
        System.out.println();
        System.out.println();

        // PRINT ASSIGNMENT RESULTS
        int j = 1;
        for (List<Double> aScore : assignmentScores) {
            // converting from List<Double> to double[]
            // http://stackoverflow.com/questions/6018267/how-to-cast-from-listdouble-to-double-in-java
            double[] row = aScore.stream().mapToDouble(Double::doubleValue).toArray();  // converting to a normal array.
            System.out.println("Assignment: " + j);
            System.out.println("Highest score = " + max(row));
            System.out.println("Lowest score = " + min(row));
            System.out.println("Mean = " + mean(row) + " Grade:" + gradeLetter(mean(row)));
            System.out.println("Mean (lowest dropped) = " + meanLowDrop(row));
            System.out.println("-------------------------------------");
            j++;
        }
    }

    /**
     * get the largest number from an array of numbers.
     * @param numbers array.
     * @return double.
     */
    public static double max(double[] numbers) {
        double result = numbers[0];
        for (double number : numbers) {
            if (number > result) {
                result = number;
            }
        }
        return result;
    }

    /**
     * get the smallest number from an array of numbers.
     * @param numbers array.
     * @return double.
     */
    public static double min(double[] numbers) {
        double result = numbers[0];
        for (double number : numbers) {
            if (number < result) {
                result = number;
            }
        }
        return result;
    }

    /**
     * get the mean of all the numbers in the array.
     * @param numbers array.
     * @return double.
     */
    public static double mean(double[] numbers) {
        double sum = 0;
        for (double number : numbers) {
            sum += number;
        }
        return sum / numbers.length;
    }

    public static double meanLowDrop(double[] numbers) {
        double lowestGrade = min(numbers);
        double sum = 0;
        for (double number : numbers) {
            sum += number;
        }
        return (sum - lowestGrade) / (numbers.length - 1);
    }

    /**
     * return a grade letter based on the student's mean grade.
     * @param mean double.
     * @return char.
     */
    public static char gradeLetter(double mean) {
        char result;
        if (mean >= 90) {
            result = 'A';
        } else if (mean >= 80) {
            result = 'B';
        } else if (mean >= 70) {
            result = 'C';
        } else if (mean >= 65) {
            result = 'D';
        } else {
            result = 'F';
        }
        return result;
    }

    /**
     * access columns of a 2d array and return them as rows in an array.
     * @param studentGrades 2d array
     * @return array
     */
    public static List<List<Double>> colsToRows(double[][] studentGrades) {
        List<List<Double>> rows = new ArrayList<>();
        for (int i = 0; i < studentGrades.length; i++) {
            List<Double> temp = new ArrayList<>();
            for (double[] studentGrade : studentGrades) {
                temp.add(studentGrade[i]);
            }
            rows.add(temp);
        }
        return rows;
    }
}
\$\endgroup\$
8
  • \$\begingroup\$ Why not just return 'A'; etc. in gradeLetter rather than setting up a variable to return at the end? \$\endgroup\$ Dec 21, 2015 at 9:51
  • \$\begingroup\$ One of my coding buddies told me once that the code is more readable generally when it simply has one return statement. \$\endgroup\$ Dec 21, 2015 at 10:28
  • \$\begingroup\$ I guess it's an opinion which is better, I like that there's less lines and since all the different conditions would look so similar I'd find it readable. But I don't know of any actual benefit apart from it being more readable (to me). \$\endgroup\$ Dec 21, 2015 at 10:31
  • \$\begingroup\$ Yeah i could see that. What you are saying makes sense. By the way for my grade letter method... Should I put the if and else if statements logic one line or leave it as is? Which is more idiomatic in Java? \$\endgroup\$ Dec 21, 2015 at 10:35
  • \$\begingroup\$ If you mean having if (mean >= 90) { result = 'A'; } all on one line then I'd definitely leave it as is. It's almost never a good idea to put condition and code block on one line like that. \$\endgroup\$ Dec 21, 2015 at 10:36

2 Answers 2

4
\$\begingroup\$

I need particular feedback on the use of my ArrayList in my colsToRows method and then how I converted it back to regular arrays in Java. What better step could I have taken there?

The better step could be to just stick with either arrays or ArrayList, if that's feasible. Converting between them is adding complexity to your code. If studentScores was defined as List<List<Double>>, you could use the existing Collections.max and .min methods:

for (List<Double> scores : studentScores) {
    ...
    System.out.println("Highest score = " + Collections.max(scores));
    ...
}

Ignoring that though, your code is quite clean and readable.

Your max and min methods are perfectly idiomatic, although you may want to check that the array passed into them has .length > 0 before attempting to access index 0.

Defining a sum method could help reduce code duplicated between mean and meanLowDrop. You may also want to handle the case of empty arrays or of meanLowDrop receiving an array of size 1:

public static double mean(double[] numbers) {
    if (numbers.length == 0) {
        throw new IllegalArgumentException("Cannot take mean of empty array.");
    }
    return sum(numbers) / numbers.length;
}

public static double meanLowDrop(double[] numbers) {
    if (numbers.length == 0) {
       throw new IllegalArgumentException("Cannot take mean of empty array.");
    } else if (numbers.length == 1) {
        return mean(numbers);
    }
    return (sum(numbers) - min(numbers)) / (numbers.length - 1);
}

I might also rename meanLowDrop to meanDropLowest or meanWithoutLowest.

gradeLetter is perfect.

You could rewrite colsToRows to directly return a double[][] instead of a List<List<Double>>, to avoid the dance with .stream().mapToDouble... that you're currently doing when printing:

public static double[][] colsToRows(double[][] arr) {
    double[][] transposed = new double[arr.length][arr[0].length];

    for (int i = 0; i < arr.length; i++) {
        for (int j = 0; j < arr[0].length; j++) {
            transposed[i][j] = arr[j][i];
        }
    }
    return transposed;
}

And as one last point, moving most of main into a separate method that looks something like this:

public static printStats(double[][] studentScores, String[] studentNames)

would make it easier to reuse your code with different students, to run tests, and so on.

\$\endgroup\$
3
  • \$\begingroup\$ What if max was not built in to collections? Like my mean or my lowest grade dropped function? Then how do I avoid the code complexity as you mentioned of converting from list double to double []? \$\endgroup\$ Dec 21, 2015 at 10:31
  • \$\begingroup\$ @Rafeh I just meant that there are two options - make everything lists (you can use Collections.max/min and your mean, etc would be written to take List) or use only arrays, like you've done, in which case colsToRows should be rewritten to return an array instead of a List. If everything else is double[][], then colsToRows returning a List is the only thing adding complexity. \$\endgroup\$
    – BenC
    Dec 23, 2015 at 0:10
  • \$\begingroup\$ Ah this is perfect! This makes a lot of sense! Transposed method returning a regular double[] was exactly what I needed in order to reduce the code complexity. I have also added the printStats method now. I really appreciate this, this is awesome! \$\endgroup\$ Dec 23, 2015 at 21:22
1
\$\begingroup\$

Input validation

While I agree with @BenC that the mean and meanLowDrop methods need some protection against empty array parameters, this check should be at a higher level, for example in this method:

public static printStats(double[][] studentScores, String[] studentNames) {
    if (studentScores.length == 0) {
        throw new IllegalArgumentException("Student scores must not be empty.");
    }
    // ...

In the same method, another check is due:

    if (studentScores.length == studentNames.length) {
        throw new IllegalArgumentException("There must be the same number of student scores and student names.");
    }

Aggregate easier using Java 8

Java 8 makes it easier to calculate aggregates, for example:

// max
return DoubleStream.of(numbers).max().getAsDouble();

// sum
return DoubleStream.of(numbers).sum();

... and so on, you get the idea.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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