Try catch block added #90
Conversation
WalkthroughThe processing logic for each patient in the Changes
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/main/java/com/wipro/fhir/service/common/CommonServiceImpl.java (2)
176-179: Approve the resilience improvement but suggest logging enhancement.The addition of the outer try-catch block successfully prevents individual patient processing failures from stopping the entire batch operation, which improves system resilience.
Consider enhancing the error logging to include patient identification details:
- for (PatientEligibleForResourceCreation p : pList) { - try { - logger.info("Bundle creation is started for BenID : " + p.getBeneficiaryId() + " | BenRegID : " - + p.getBeneficiaryRegID() + " | VisitCode : " + p.getVisitCode()); + for (PatientEligibleForResourceCreation p : pList) { + try { + logger.info("Bundle creation is started for BenID : " + p.getBeneficiaryId() + " | BenRegID : " + + p.getBeneficiaryRegID() + " | VisitCode : " + p.getVisitCode());
237-239: Fix logging message formatting.The error logging message has a formatting issue that affects readability.
- } catch (Exception e) { - logger.error("Fhir Schedule run failed " +e.getMessage()); - } + } catch (Exception e) { + logger.error("FHIR Schedule run failed for BenID: " + p.getBeneficiaryId() + + " | BenRegID: " + p.getBeneficiaryRegID() + + " | VisitCode: " + p.getVisitCode() + " - " + e.getMessage(), e); + }Improvements:
- Fixed spacing in the log message
- Added patient context for easier debugging
- Included the full exception stack trace for better troubleshooting
- Corrected "Fhir" to "FHIR" (standard capitalization)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/wipro/fhir/service/common/CommonServiceImpl.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (2)
src/main/java/com/wipro/fhir/service/common/CommonServiceImpl.java (2)
212-234: Verify error handling strategy aligns with business requirements.The bundle processing logic (OP consult, diagnostic report, prescription) now continues execution even if the demographics fetching or SMS sending fails due to the nested exception handling. This might not be the intended behavior.
Please verify whether the bundle processing should continue if demographics fetching fails. The current implementation will:
- Continue processing bundles even if
addCareContextToMongo()fails- Continue processing bundles even if patient demographics cannot be fetched
- Only skip SMS sending if phone number is missing
Is this the intended behavior, or should demographic fetching failures prevent bundle processing?
186-209: 🛠️ Refactor suggestionRefactor nested exception handling for better maintainability.
The nested try-catch structure creates complexity and potential for unexpected behavior. The inner catch block silently swallows exceptions, which could mask important failures during demographics fetching or SMS sending.
Consider refactoring to a more straightforward approach:
- try { - - logger.info("*****Fetch beneficiary Id: " + resourceRequestHandler.getBeneficiaryRegID()); - List<Object[]> rsObjList = patientEligibleForResourceCreationRepo - .callPatientDemographicSP(resourceRequestHandler.getBeneficiaryRegID()); - logger.info("*****Fetch beneficiary Id response recevied :", rsObjList); - - PatientDemographic patientDemographicOBJ = patientDemographic.getPatientDemographic(rsObjList); - logger.info("*****Fetch patient after fetching demographics"); - if (patientDemographicOBJ != null) { - addCareContextToMongo(patientDemographicOBJ, p); - logger.info("*****Add to mongo success done"); - if (patientDemographicOBJ.getPreferredPhoneNo() != null) - sendAbdmAdvSMS(patientDemographicOBJ.getPreferredPhoneNo()); - else - throw new FHIRException( - "Advertisement sms could not be sent as beneficiary phone no not found"); - } else - throw new FHIRException( - "Beneficiary not found, benRegId = " + resourceRequestHandler.getBeneficiaryRegID()); - - } catch (Exception e) { - logger.error(e.getMessage()); - } + logger.info("*****Fetch beneficiary Id: " + resourceRequestHandler.getBeneficiaryRegID()); + List<Object[]> rsObjList = patientEligibleForResourceCreationRepo + .callPatientDemographicSP(resourceRequestHandler.getBeneficiaryRegID()); + logger.info("*****Fetch beneficiary Id response recevied :", rsObjList); + + PatientDemographic patientDemographicOBJ = patientDemographic.getPatientDemographic(rsObjList); + logger.info("*****Fetch patient after fetching demographics"); + if (patientDemographicOBJ != null) { + addCareContextToMongo(patientDemographicOBJ, p); + logger.info("*****Add to mongo success done"); + if (patientDemographicOBJ.getPreferredPhoneNo() != null) { + sendAbdmAdvSMS(patientDemographicOBJ.getPreferredPhoneNo()); + } else { + logger.warn("Advertisement SMS could not be sent as beneficiary phone no not found for BenRegID: " + + resourceRequestHandler.getBeneficiaryRegID()); + } + } else { + logger.error("Beneficiary not found, benRegId = " + resourceRequestHandler.getBeneficiaryRegID()); + }This approach:
- Removes the nested try-catch complexity
- Allows the outer try-catch to handle all exceptions consistently
- Changes the missing phone number from an exception to a warning (since SMS is auxiliary functionality)
- Improves error messages with context
Likely an incorrect or invalid review comment.
2 participants
📋 Description
JIRA ID:
Please provide a summary of the change and the motivation behind it. Include relevant context and details.
✅ Type of Change
ℹ️ Additional Information
Please describe how the changes were tested, and include any relevant screenshots, logs, or other information that provides additional context.
Summary by CodeRabbit