Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add mpsii comments mapping refactor #1477

Merged

Conversation

jorg3lopez
Copy link
Contributor

#Refactoring of original PR (Add mpsii comments mapping refactor)

Describe what changed in this PR at a high level.

jorg3lopez and others added 3 commits October 22, 2024 10:55
use stream instead of for loop
use IllegalArgumentException instead of NullPointerException
make tests pass
add helper method isLocalCode
add helper method processCoding
add helper method validateField
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Exception Handling
The change from NullPointerException to IllegalArgumentException for missing fields should be validated to ensure it aligns with the overall error handling strategy of the application.

@@ -28,43 +28,63 @@ public void transform(HealthData<?> resource, Map<String, Object> args) {
var codingMap = getMapFromArgs(args);

var bundle = (Bundle) resource.getUnderlyingData();
var msh41Identifier = extractMsh41Identifier(bundle);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding a null check for bundle in extractMsh41Identifier method to prevent potential NullPointerExceptions. This is important as the method assumes bundle is not null. [important]

observation.getCode().getCoding().add(0, mappedCoding);
}

private String validateField(String field, String fieldName) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactor the validateField method to handle different types of validation based on the field type or requirements. This could improve the flexibility and reusability of the validation logic. [medium]


private String validateField(String field, String fieldName) {
if (field == null || field.isBlank()) {
throw new IllegalArgumentException("missing or empty required field " + fieldName);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using a more specific exception than IllegalArgumentException for different validation failures in validateField, such as MissingFieldException or InvalidFieldException, to provide clearer error information. [important]

Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Score
Possible bug
Add a null check for bundle to prevent potential runtime exceptions

Consider adding a null check for bundle in the extractMsh41Identifier method to
prevent potential NullPointerException when accessing methods on bundle.

etor/src/main/java/gov/hhs/cdc/trustedintermediary/etor/ruleengine/transformation/custom/MapLocalObservationCodes.java [53]

+if (bundle == null) throw new IllegalArgumentException("Bundle cannot be null.");
 var msh41Identifier = HapiHelper.getMSH4_1Identifier(bundle);
Suggestion importance[1-10]: 7

Why: Adding a null check for bundle in the extractMsh41Identifier method is a prudent safeguard. It prevents potential NullPointerException which could occur if bundle is null, thus enhancing the robustness and reliability of the code.

7
Enhancement
Enhance the exception message for better clarity and debugging

Update the exception message to include more context about the missing field, which
can aid in debugging and user error resolution.

etor/src/test/groovy/gov/hhs/cdc/trustedintermediary/etor/ruleengine/transformation/custom/MapLocalObservationCodesTest.groovy [249]

-def exceptionMessage = "missing or empty required field codingSystem"
+def exceptionMessage = "Error: The required field 'codingSystem' is missing or empty."
Suggestion importance[1-10]: 6

Why: Enhancing the exception message to provide more context is a valid improvement. It aids in debugging and provides clearer error information to the user, making the system more user-friendly and maintainable.

6
Best practice
Use a more specific exception type for field validation to improve error clarity

Refactor the validateField method to throw a more specific exception such as
MissingFieldException instead of IllegalArgumentException to provide clearer
semantics.

etor/src/main/java/gov/hhs/cdc/trustedintermediary/etor/ruleengine/transformation/custom/MapLocalObservationCodes.java [76]

-throw new IllegalArgumentException("missing or empty required field " + fieldName);
+throw new MissingFieldException("missing or empty required field " + fieldName);
Suggestion importance[1-10]: 4

Why: While using a more specific exception type like MissingFieldException could improve error clarity, the suggestion is not actionable without defining such an exception class. The current use of IllegalArgumentException is standard and acceptable, though the suggestion has merit in terms of best practices for clarity.

4

@jorg3lopez jorg3lopez merged commit 2a961d4 into add-mpsii-comments-mapping Oct 23, 2024
2 checks passed
@jorg3lopez jorg3lopez deleted the add-mpsii-comments-mapping-refactor branch October 23, 2024 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant