-2
\$\begingroup\$

Student.java

public class Student{
    /*
    Metrics examples (out of 100):
    mathScore:98
    scienceScore:99
    englishScore:92
    */
    private Map<String, Double> metrics = new HashMap<>();

    public Double getMetric(final String metricName) {
     return metrics.get(metricName);
   }

}

Statistics

public enum Statistic {

     AVERAGE,
     MIN,
     MAX
}

Result

public class Result {
  private Statistic statistic;
  private double value;
  private String metric;
}

What's the most efficient way to build an engine that can perform requested statical analysis based on the requested metrics on a list of students

Here's an example:

Say I have a list of Students.

List<Student> students = [john, json, elliot, sarah, callie, matt, leigh]; //Assume each entry is an object 

I also have list of metrics I'm interested in

List<String> metrics  =["mathScore", "scienceScore"]; //Just an example. I can add additional metrics to this list or remove them.

And the statistics I want to perform

List<Statistic> stats = [MIN, MAX]; //Just an example. I can request additional operations if necessary.

Here's the signature of the method that needs to be efficiently built

public List<Result> calculate( List<String> requestedMetrics, List<Statistic> requestedStatistics, List<Student> students){

}

Here are my initial thoughts

  1. Convert requestedMetrics and requestedStatistics to sets to eliminate duplicates.
  2. Iterate through each metric. For each metric, iterate through each statistic and calculate it. Is there a better way? And how does one go about breaking down the implementation in to smaller functions etc. for a cleaner solution?
  3. What about creating a cache (map) so that we don't need to reprocess everything again and again?

Here's my current implementation

@Component
public class StatisticalAnalysis {

    @Override
    public List<Result> calculate(List<Student> students, List<String> metrics, List<Statistic> stats) {

        return analyze(new HashSet<>(students), new HashSet<>(metrics), new HashSet<>(stats));
    }

    public List<Result> analyze(HashSet<Student> students, HashSet<String> metrics, HashSet<Statistic> stats) {

        List<Result> results = new ArrayList<>(metrics.size());

        for (String metric : metrics) {
            for (Statistic stat : stats) {
               results.add(createResult(students, metric, stat ));
            }
        }

        return results;
    }

    private Result createResult(HashSet<Student> students, String metric, Statistic stat) {

       return new Result(metric, stat, calcStatValue(students, metric, stat));

    }

    private double calcStatValue(HashSet<Student> students, String metric, Statistic stat) {


        List<Double> values = new ArrayList<Double>(students.size());

        for(Student measurement: students){
            Double value = measurement.getMetric(metric);
            if(value!=null)
                values.add(value);
        }

        return performStatOperation(stat, values);

    }

    private double performStatOperation(Statistic stat, List<Double> values) {
        switch (stat) {
            case MIN:
                return Collections.min(values);
            case MAX:
                return Collections.max(values);
            case AVERAGE:
                return values.stream().mapToDouble(val -> val).average().orElse(0.0);
            default:
                throw new UnsupportedOperationException(String.format("Calculation of Statistic %s is currently unsupported", stat));
        }
    }


}
\$\endgroup\$

closed as off-topic by πάντα ῥεῖ, Mast, Toby Speight, Donald.McLean, AJNeufeld Jul 29 at 17:17

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Code not implemented or not working as intended: Code Review is a community where programmers peer-review your working code to address issues such as security, maintainability, performance, and scalability. We require that the code be working correctly, to the best of the author's knowledge, before proceeding with a review." – Toby Speight, Donald.McLean
If this question can be reworded to fit the rules in the help center, please edit the question.

  • \$\begingroup\$ @dfhwze Removed it. One of the goals of the review is to see if there's another style that's more efficient. \$\endgroup\$ – user6123723 Jul 28 at 18:28
  • \$\begingroup\$ @dfhwze I can't specifically think of individual algorithmic improvements. But there could be enhancements that I'm missing. Also is there an alternative design style and how does that compare? Is there a disadvantage to creating smaller functions? These are just a few examples. There could be other elements to review! \$\endgroup\$ – user6123723 Jul 28 at 18:34
  • 1
    \$\begingroup\$ Added! Thank you! \$\endgroup\$ – user6123723 Jul 28 at 19:10
  • 1
    \$\begingroup\$ Shouldn't List<Result> calculate be List<Result> result? \$\endgroup\$ – vnp Jul 28 at 20:05
  • 3
    \$\begingroup\$ It's hard to believe that this code works as expected. You cannot get any information from the Student class since the map of metrics is initially empty and not modifiable from outside the Student class. \$\endgroup\$ – Roland Illig Jul 28 at 20:27
2
\$\begingroup\$

This can be vastly simplified using a handful of tricks from functional programming. Let's first consider the operations you want to perform, given a collection of students, metrics and stats.

  1. Extract the metrics you're interested in from the students. In SQL this would be equivalent to a SELECT clause.
  2. Perform the calculations for each of the statistics you want on the metrics you obtained.
  3. Return a Result for each combination of metrics and stats with the calculated value.

Right now performing the calculation is something that calcStatValue knows about. This makes it a bit annoying to add an additional statistic like MEDIAN, because you need to track down the switch-case statement.

To avoid this, the calculation should be the responsibility of the statistic itself. Given that you can have members on Enums, the following should work:

public enum Statistic {
    MIN(values -> Collections.min(values)),
    MAX(values -> Collections.max(values)),
    AVERAGE(values -> values.stream().mapToDouble(v -> v).average().orElse(0.0));

    private final Function<Collection<Double>, Double> aggregator;

    private Statistic(Function<Collection<Double>, Double> aggregator) {
        this.aggregator = aggregator;
    }

    public double performStatOperation(Collection<Double> values) {
        return aggregator.apply(values);
    }
}

The rest of my suggestions basically boils down to some minor changes:

  • If you preinitialize the size of results in analyze, you should use the actual number of slots you'll be using with new ArrayList<>(metrics.size() * stats.size()).
  • You don't gain significant benefits from converting the List parameters to HashSets. If you want to force uniqueness in the implementation, you really should be communicating that in the public API of your class by only accepting Sets in the first place.
  • calcStatValue can be simplified using streams:

    return stat.performStatOperation(students.stream()
      .map(s -> s.getMetric(metric))
      .collect(Collectors.toSet()));
    
  • A similar simplification applies to analyze, but I'll not write that out here.

Additional remarks:

  • The results.add([..]) line in analyze has inconsistent spacing at the parentheses.
  • Using a specific collection implementation as API (i.e. HashSet as parameter) should be avoided.
  • The domain model representation sucks. Representing a metric by a String is not a good idea (because tyops), and forcing each metric to be represented in a double is also bad (because non-numeric metrics are a thing). In addition your API currently does not have any way to perform aggregation and analysis operations that do not yield a single result (e.g. histogramming, tallying, ...)
\$\endgroup\$

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