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;
}
}
2 Answers 2
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.
-
\$\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\$Clever Programmer– Clever Programmer2015年12月21日 10:31:41 +00:00Commented 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 yourmean
, etc would be written to takeList
) or use only arrays, like you've done, in which casecolsToRows
should be rewritten to return an array instead of a List. If everything else isdouble[][]
, thencolsToRows
returning aList
is the only thing adding complexity. \$\endgroup\$BenC– BenC2015年12月23日 00:10:58 +00:00Commented 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\$Clever Programmer– Clever Programmer2015年12月23日 21:22:52 +00:00Commented Dec 23, 2015 at 21:22
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.
return 'A';
etc. ingradeLetter
rather than setting up a variable to return at the end? \$\endgroup\$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\$