junaid
March 15, 2026, 3:42am
1
hello all , can you please review my PR
opened 06:10PM - 12 Mar 26 UTC
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.
opened 04:51AM - 08 Mar 26 UTC
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.
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.
junaid
March 16, 2026, 3:01pm
3
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