Basic statistics implementation, not yet integrated.#3
Basic statistics implementation, not yet integrated.#3stopping wants to merge 1 commit intocleichner:masterfrom
Conversation
individual courses and aggregating data for multiple courses (e.g. ranking).
There was a problem hiding this comment.
got a better name than ConcreteAggregator?
There was a problem hiding this comment.
My personal convention recently has been to name classes 'Standard[interface name]" for interfaces which only have one real implementation (not including test classes). Any better alternatives?
There was a problem hiding this comment.
For [interface] and single-implementation, I prefer the "[Interface]Impl" convention. But ideally I'd prefer a descriptive name, particularly since it's an "aggregator" and should describe what kind of aggregation it is doing. RankingDataAggregator?
Note that if you don't have a reason to split the interface/implementation out, you can also just combine them into one class. Guice isn't restricted to injecting interfaces, so injecting a concrete class could make sense if you don't expect to have other aggregators going forward.
There are definitely a few cases in the codebase right now of an interface with a single implementation, but most of those have an intention (or at least a plausible possibility) to have multiple future implementations.
There was a problem hiding this comment.
There are definitely a few cases in the codebase right now of an interface with a single implementation, but most of those have an intention (or at least a plausible possibility) to have multiple future implementations.
Also unit tests. :)
There was a problem hiding this comment.
Maybe we can change DataAggregator so that it just implements a single "aggregate" method. This way we can have sorting aggregators, grouping aggregators, etc. Maybe genericize DataAggregator with the return type of the aggregate method?
public class RankingDataAggregator implements DataAggregator<List>
There was a problem hiding this comment.
That seems reasonable, what would the return type be though? Why not always
a Rateable?
On Tue, May 5, 2015 at 12:09 PM, Sean Topping notifications@github.com
wrote:
In src/main/java/com/whichclasses/statistics/ConcreteAggregator.java
#3 (comment):+package com.whichclasses.statistics;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+
+import com.google.common.collect.Maps;
+import com.google.inject.Inject;
+import com.whichclasses.model.Course;
+import com.whichclasses.model.DataSource;
+import com.whichclasses.model.Department;
+import com.whichclasses.model.proto.TceRating.Question;
+
+public class ConcreteAggregator implements DataAggregator {Maybe we can change DataAggregator so that it just implements a single
"aggregate" method. This way we can have sorting aggregators, grouping
aggregators, etc. Maybe genericize DataAggregator with the return type of
the aggregate method?public class RankingDataAggregator implements DataAggregator>
—
Reply to this email directly or view it on GitHub
https://github.com/cleichner/whichclasses/pull/3/files#r29702022.
There was a problem hiding this comment.
I guess the intent of the class is to answer queries such as:
"Give me a list of rated courses in CSC."
"Give me a list of rated professors in AME."
"Give me a list of rated semesters for ACCT." (how are the classes in ACCT changing over time?)
Come to think of it, maybe the aggregator should always just return a list of Rateable objects. Then you can do sorting and selection as needed. Basically it's just a way to convert a large collection of individual class ratings into chunked rated collections (e.g. courses, departments, instructors, semesters, etc). Basically a GROUP BY implementation.
Implementation of statistics objects, including methods for scoring individual courses and aggregating data for multiple courses (e.g.
ranking).