search

comunica / sparqlee

⚙️ SPARQL expression evaluator library - Moved to @comunica/expression-evaluator
https://www.npmjs.com/package/sparqlee
14 stars 7 forks source link

AggregateEvaluators is a mess #90

Closed jitsedesmet closed 3 years ago

jitsedesmet commented 3 years ago

AggregateEvaluators is a mess, the single file is 400 lines long and contains 11 classes. If you ask me, we should clean this up by making a new folder aggregators or something like that. And give these classes their own file. I know this will result in a lot of small files but it's cleaner in my opinion.

wschella commented 3 years ago

Splitting all logic without discrimination, while not adding extra documentation or reducing complexity IMO just hides the dirtyness instead of cleaning it up. So I don't really see much advantage (as the issue is posed right now), as this code is very tightly coupled.

Also, 7 of the 11 classes are simply extensions of BaseAggregator, implementing the various aggregate functions Max, Min, GroupConcat... I guess it could make sense to split those off (together with BaseAggregator), but I would keep those in a single file.

The other ones could be split to have symmetry with the SyncEvaluator & AsyncEvaluator, but I would seriously consider removing the abstract class in favor of a bit more code/doc duplication. That would make 3 files total, with decent logic boundaries.

Does that plan make sense?

jitsedesmet commented 3 years ago

Yes, I think it makes sense to keep the Aggregators together in 1 file. I don't really like the idea of removing the abstract class. I personally think we should keep sync and async together where we can. If we were to just split those up entirely we might as well make a package aync-sparqlee and sync-sparqlee (this is a joke :p ). We could just have 4 files (1 extra for the abstract class). Let me know what you think! :D

wschella commented 3 years ago

Okay!

rubensworks commented 3 years ago

FYI, it is typically considered a good practise to have separate files for each class. That said, there is indeed a case to be made for placing highly-similar and small classes within a single file, such as the aggregators. I would personally place them in separate files, but in a separate package/directory, but that's just my opinion.

While I write this, I realized that we even have an eslint rule for this, so I've created an issue for migrating to eslint: https://github.com/comunica/sparqlee/issues/93