Reduce code duplication in MCCL detectors

[hayakawa16]

Currently when new detectors are added, the duplication in new code is ~33% and does not pass the SonarCloud build and analyze quality review.

Reduce code duplication by creating Base classes for types of detectors (like what is done for Sources). This could be done by general groups (reflectance/transmittance or 1D/2D). Not sure until code is analyzed for best approach.

Also, possibly group detectors into folders once segmenting is performed (like how Sources are organized).

Currently we increase the check limit to 33% in order to pass quality check, then reduce back after branch is merged.

[dcuccia]

This was by design to reduce coupling between classes and use the same pattern an external developer would use to build a detector. If they're trying to copy-paste the implementation, it's going to be a lot more cognitive load to understand inheritance and/or factory methods. On that last point - if we did go this direction, I would heavily favor static factory helper classes (like we do for some source distributions? can't remember...) over inheritance and base classes.

[hayakawa16]

Hi @dcuccia, thanks for your response! I have converted this to a discussion because it wasn't set that a change would necessarily be invoked. Just wanted to discuss potential options. Please provide more details about your thoughts.

[dcuccia]

Thanks Carole. What would best to expound on, re: the above comments?

[hayakawa16]

Hi @dcuccia, I'm going back and forth on this so my comments below may be scattered. Just trying to feel my way to the best implementation...

I remember having a discussion with you way back and I asked about putting common code across detectors into a class and having the detectors refer to that code. I remember you saying that the it would be best design if each detector class housed all it needs. This is good in case particulars to a few detectors arise. However it means that large code sections are repeated across certain detectors. I appreciate your comment about copy-paste. That is pretty much how new detectors are created. Its easy and works. Recently I added Lambertian classes to certain sources which have Base classes (see below) and it took a bit to determine if I had to modify the Base class to have a "protected abstract Direction GetFinalDirection" because a derived class needed a different implementation. So yes using a Base class requires the coder to know and understand more about the other derived classes and how code changes are affecting them. I see your hesitation here.

In the sources we have Base classes for each kind, e.g. Line, Point, etc. We also have SourceToolbox that contains static methods that all sources can use. This makes sense for sources because these are algorithmic sampling distributions (e.g. Gaussian). I'm not sure what static methods could be defined for detectors. Could you expand on your concept here?

[dcuccia]

Right, I think the point of having "built-in" detectors should be that they are stand-alone templates that users can take and vamp off of. The programming model here is that users shouldn't have to recompile Vts.dll to use their own detector. They would copy one that was close to what they wanted, and edit/augment as they saw fit, and then use that detector without that code depending on some other base class somewhere in Vts.dll. See here, for example (same thing with forward solvers, etc). In summary, we are designing for API use by the person consuming Vts.dll. Tools like SonarCloud are great, but their default rules shouldn't be a reason to degrade the onboarding/understandability by our target users. How many problems have we had where shared detector code caused us an actual headache? I would weigh that, and the knowledge that we've had a pretty stable codebase for a long time, against the benefit of maintaining/fixing shared detector bugs.

And thanks, the SourceToolbox classes are what I was thinking about - static helper methods, vs complex inheritance/code ownership. If I were to re-design sources and code sharing based on my latest 10 years of developer experience, I'd also lean away from shared base classes there, too, and add more toolbox code. The flatter the better - many tools/frameworks these days don't even require you to implement an interface and do more of what's called "duck typing" - e.g. as long as a class has methods/properties with the correct naming/parameter signature, they can get automatically be used by the machinery.

At the risk of confusing the point of this specific discussion, I'd say generally my vision for a redesign (or mild refactoring) would be to make the core engine lighter weight. The Vts.MonteCarlo core would itself not have any detectors/sources/tissues - it would be solely the engine that would work with any/all detectors/tissues/sources. We could then have a separate Vts.MonteCarlo.Sources package for those that wanted the samples (but it wouldn't be necessary for the engine to do its work). Of course, validation code appropriate to a particular kind of source would need to be packaged alongside it, but this would encourage good encapsulation - no more laundry lists of pre-fixed types of functionality, but an open and expanding set of capabilities. Experts could publish their own sources/detectors/tissues, which would be compatible with the core interfaces, and users anywhere could bring in and use those, too.