# Review request PR : #3018 and #30360

**URL:** <https://talk.openelis-global.org/t/review-request-pr-3018-and-30360/2106>\
**Category:** Uncategorized\
**Created:** [March 15, 2026, 3:42am UTC](https://talk.openelis-global.org/t/review-request-pr-3018-and-30360/2106 "2026-03-15T03:42:52Z")\
**Posts on this page:** 3\
**Page:** 1

<div class="post-metadata">

**Author:** ![junaid](https://yyz2.discourse-cdn.com/flex030/user_avatar/talk.openelis-global.org/junaid/32/1166_2.png) [@junaid](https://talk.openelis-global.org/u/junaid)\
**Post date:** [March 15, 2026, 3:42am UTC](https://talk.openelis-global.org/t/review-request-pr-3018-and-30360/2106/1 "2026-03-15T03:42:52Z")

</div>

hello all , can you please review my PR

> <https://github.com/DIGI-UW/OpenELIS-Global-2/issues/3060>
>
> While exploring the reporting module, I noticed that \`ReportImplementationFactor…y\` currently uses a long chain of \`if/else\` statements to map report identifiers to their corresponding implementations of \`IReportCreator\` and \`IReportParameterSetter\`.
> 
> Since there are many report implementations (60+ classes under \`reports/action/implementation\`), this approach makes the factory difficult to maintain and requires modifying it each time a new report is introduced.
> 
> Example pattern currently used:
> 
> \`\`\`
> if (reportName.equals("PatientClinicalReport")) {
> return new PatientClinicalReport();
> } else if (reportName.equals("IndicatorHIV")) {
> return new IndicatorHIV();
> }
> ...
> \`\`\`
> 
> \### Possible Improvement
> 
> Spring already supports automatic discovery of beans implementing a common interface. Instead of hardcoding these mappings in the factory, we could leverage Spring’s dependency injection to register report implementations dynamically.
> 
> Conceptually this could look like:
> 
> \`\`\`
> @Autowired
> private Map\<String, IReportCreator\> reportCreators;
> 
> @Autowired
> private Map\<String, IReportParameterSetter\> parameterSetters;
> \`\`\`
> 
> Each report implementation could be annotated with \`@Component("reportId")\`, allowing the factory to retrieve the correct implementation directly from the injected map.
> 
> Example usage:
> 
> \`\`\`
> public IReportCreator getReportCreator(String reportId) {
> return reportCreators.get(reportId);
> }
> \`\`\`
> 
> \### Potential Benefits
> 
> \* Eliminates large \`if/else\` chains in \`ReportImplementationFactory\`
> \* Makes the system easier to extend when adding new reports
> \* Aligns the implementation with standard Spring dependency injection patterns
> \* Reduces coupling between the factory and individual report classes
> 
> \### Question for Maintainers
> 
> If there are plans to transition the reporting system toward a more dynamic or metadata-driven architecture, this refactoring might not be necessary.
> 
> I would appreciate guidance from maintainers on whether this refactor would be a useful improvement for the current architecture. If so, I would be happy to work on a PR for it.

> <https://github.com/DIGI-UW/OpenELIS-Global-2/issues/3018>
>
> i have noticed 4 classes are using the class.newInstace() which can be safely r…eplace with getDeclaredConstructor().newInstance() without changing any behaviour which is recommended as per java 21 
> 
> if using of class.newInstance() was not intentional then i can submit a PR with refactored changes which i had already changed and tested locally 
> 
> thank you

thank you.

---

<div class="post-metadata">

**Author:** ![priyanshu56](https://yyz2.discourse-cdn.com/flex030/user_avatar/talk.openelis-global.org/priyanshu56/32/1148_2.png) [@priyanshu56](https://talk.openelis-global.org/u/priyanshu56)\
**Post date:** [March 15, 2026, 3:28pm UTC](https://talk.openelis-global.org/t/review-request-pr-3018-and-30360/2106/2 "2026-03-15T15:28:24Z")

</div>

Nice refactor removing the large `if/else` chain and moving to a Spring-based registry — this improves maintainability and extensibility.

One concern: `ReportRegistry#getReportCreator` currently relies on catching `BeansException` for control flow, which may introduce unnecessary overhead.

It might be safer to check bean existence (`containsBean` / `isTypeMatch`) before calling `getBean`. Also worth verifying that prototype-scoped report beans don’t trigger heavy `@PostConstruct` work on every instantiation.

---

<div class="post-metadata">

**Author:** ![junaid](https://yyz2.discourse-cdn.com/flex030/user_avatar/talk.openelis-global.org/junaid/32/1166_2.png) [@junaid](https://talk.openelis-global.org/u/junaid)\
**Post date:** [March 16, 2026, 3:01pm UTC](https://talk.openelis-global.org/t/review-request-pr-3018-and-30360/2106/3 "2026-03-16T15:01:06Z")

</div>

yes i’ll fix the exceptions handling one with pre checking and yeah for the second one i got the same doubt but i though singleton bean may have risk of mixing up previous request data incase if any field was null , so can u please clarify on this , thanks for the reply it means alot