eic / EICrecon

EIC Reconstruction - JANA based
https://eic.github.io/EICrecon
GNU Lesser General Public License v3.0
6 stars 29 forks source link

EICrecon violates DRY in collection name definitions and use #537

Open wdconinc opened 1 year ago

wdconinc commented 1 year ago

Is your feature request related to a problem? Please describe. Output collection names are specified in two places and must be updated in two places. This leads to potential for inconsistencies, and confusion with new developers who write a factory and don't see the output in the output file.

Example: https://github.com/eic/EICrecon/blob/main/src/detectors/BEMC/CalorimeterHit_factory_EcalBarrelImagingRecHits.h#L14 https://github.com/eic/EICrecon/blob/main/src/services/io/podio/JEventProcessorPODIO.cc#L145

Describe the solution you'd like Should only need to define a collection name once. Output should be an exclude list, not an include list.

DraTeots commented 1 year ago

https://gordonc.bearblog.dev/dry-most-over-rated-programming-principle/

DraTeots commented 1 year ago

Also. While I hate how calorimetry factory looks but:

  1. https://github.com/eic/EICrecon/blob/main/src/detectors/BEMC/CalorimeterHit_factory_EcalBarrelImagingRecHits.h#L14 defines the name

  2. https://github.com/eic/EICrecon/blob/main/src/services/io/podio/JEventProcessorPODIO.cc#L145 uses the defined name

So I don't see a violation of DRY here

DraTeots commented 1 year ago

Oh yes, and IMO that list in JEventProcessorPODIO.cc is bad too. Hope that will look differently in new shiny life after refactoring/new-framework/name-it-how-you-like-it

DraTeots commented 1 year ago

And don't get me wrong here, I agree it could be made better and understand what you actually mean. Remaking it in current design would be implementing those unnecessary complexities that the article above says about. New algorithms library takes this in the account (but no, there still will be 2 lists of strings anyway). But considering this issue title, it is $100 title for 25 cents issue.

wdconinc commented 1 year ago

There, I marked down the title.

kkauder commented 10 months ago

@wdconinc Looks solved to me.

wdconinc commented 10 months ago

No. Still need to specify the collection name in both the factory and the event processor.