First of all, it's very clear you have put a lot of effort on this patch. I've applied at a local working copy of functor trunk, and it is very neat indeed. I wish my patches were as good as yours
Thank you! To be honest I think the quality of my previous patch was well under average on that – I am still wondering how Simone had so much patience to actually go through it – so I thought I'd put some serious effort in making it at least readable, so at least if this patch turns out to be a dud people don't waste too much time on trying to figure it out
I'm starting to learn about aggregators now, while reading the code and documents that you wrote. So feel free to correct or comment any of my points below.
- The documentation and examples provided are very good However, I think you used <br/> to break line between paragraph, while the other examples use <p>. The visual is a bit different.
Very good point! I never picked on that myself – I'm a sucker for closing and opening tags (e.g. <p> ..... </p>) so out of sheer laziness I guess I find myself using the <br/> a lot I went through the code and replaced all the <br/> with <p> so it should be all uniform now. (Wouldn't mind a second pair of eyes to verify that I haven't missed a <br/> though, if you have the time!)
- After I read the issue and applied the patch (I had a quick view on it before, wow, so many tests I opened Eclipse and looked for the aggregator package, but it's aggregate. I think functor, generator and adapter package names follow the name of the pattern they implement. And I have seen aggregate and aggregator as the name of the pattern. What do you think?
You have a fair point, again – the "aggregate" name stems really from the first patch sent which was quite different to what "aggregator" has become in the last patch I've submitted. It somehow never crossed my mind after the refactoring(s) to change the package to "aggregator" – though now that you mentioned it, it makes perfect sense! So I went ahead and moved packages under "aggregator".
- How does an aggregator differs from the reduce function (like in map reduce)? IIUC, aggregators are like the fold function. A "nostore aggregator" is more like a fold with recursion, while the list aggregator is a fold function which stores its partial value somewhere. Maybe we could use FoldLeft with aggregators?
I think you might be right – gonna go and have a look at FoldLeft – haven't considered that approach to be honest (nor am I that familiar with the class itself – though I am aware of the whole map/reduce paradigm).
- In the o.a.c.functor.aggregate.functions package, there are several aggregator functors for Double and Integer. The name of aggregators for Double start with Double. But the name of aggregators for Integer start with Int. In o.a.c.functor.generator.util, there are LongRange and IntegerRange. So I think in this case we could put both as Int, or both as Integer. What do you think?
My laziness strikes again I don't like typing long class names – even though I'm an Eclipse user and make full usage of all the wonders of autocomplete etc, long class names don't sit well with me (showing my old age here, see, I used a lot of "dumb" editors in my time when terminals were green and black – hence my choosing Int versus Integer. However, that could be misleading : Integer suggest java.lang.Integer wheres Int in the name could suggest the primitive int.
Since we already used the convention of Integer for when java.lang.Integer is used in functor, I went and changed all the names to include Integer rather than just Int. Thanks for the heads-up on this!
- IIUC, in o.a.c.functor.aggregate there are classes for creating nostore and list aggregators. However, if I create a ArrayListBackedAggregator<T>, this aggregator extends AbstractTimedAggregator<T>. What means AbsractTimedAggregator<Integer> aggr = new ArrayListBackedAggregator<Integer>(); would be valid (not tested, but I think it works). I think these classes could be designed in some other way to avoid this scenario... just food for thought
Yes, that is the intended behaviour: in other words the implementations provided all are meant to have built-in timer support. I have defined the top-level interface Aggregator with no timer support as there might be other scenarios the Aggregator can be used in, which don't require timer support (even though that can be achieved with a null timer in AbstractTimedAggregator).
Basically this package appeared from finding myself quite often writing similar aggregators for various apps or components I was working on – and every time I needed this data to be flushed into a sink (typically logged in a log file or over a socket) regularly. As I said, this isn't to say Aggregator must be used in a timer context – hence, as I said, the Aggregator interface doesn't make any reference to that.
Do you think these interfaces / classes should be structured some other way then? (Or is it perhaps the fact the naming of classes and interfaces is it not the best?)
- AbstractTimedAggregator<T> implements TimedAggregator<T>. But in TimedAggregator#onTimer, the first parameter is an AbstractTimedAggregator. I think this could be replaced by a TimedAggregator. What do you think? Otherwise, it would be like the interface had a dependency to a class that implements it. And if another class implemented TimedAggregator, it would have a dependency to AbstractTimedAggregator too. Or if AbstractTimedAggregator was deprecated/removed, it would cause problems in TimedAggregator interface.
There is no interface TimedAggregator – it's just Aggregator and AbstractTimedAggregator implements it. The TimedAggregatorListener indeed has a dependency on the AbstractTimedAggregator class which I agree is not the best – perhaps, a reason for introducing a TimedAggregator interface and work at interface level in TimedAggregatorListener?
Though at the same time, the only reason why AbstractTimedAggregator is mentioned in TimedAggregatorListener is because the base interface Aggregator has no concept or mention of timer, so while it is possible to have TimedAggregatorListener declare a method like this:
void onTimer(Aggregator<T> aggregator, T evaluation);
It just didn't make sense to me, since like I said, Aggregator doesn't have a concept of timer.
So the more I think about it, I could introduce a TimedAggregator interface which would extend Aggregator and offer 3 other methods:
What do you think? Or should we use Aggregator in TimedAggregatorListener?
The docs, examples and coverage are great. No issues in PMD, FindBugs or Checkstyle found so far. All licenses in place, as well as package javadoc. Next time I submit a patch I hope it can as neat as yours
There is an issue in Google Guava for similar feature, have you seen it (http://code.google.com/p/guava-libraries/issues/detail?id=546)?
I wasn't aware of it to be honest. I'll try to have a look at it but to be honest, I fear timing is not my most abundant resource and I'd rather spend most of the time coding functor I'll have a look though, just to see what they came up with.
I'm attaching an update to the patch (labelled functor.22-may-2012.patch.bz2) with the above changes – would appreciate your comments on this!