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

APCV-1367 and APCV-1268 - CS conformance issue fix #161

Merged
merged 2 commits into from
Sep 24, 2024

Conversation

palatsangeetha
Copy link
Collaborator

-Fix for incorrect prompt and updated the GET actions title for pagination scenarios

@jkosternl jkosternl changed the title APCV-1367 and APCV-1268- CS conformance issue fix APCV-1367 and APCV-1268 - CS conformance issue fix Sep 23, 2024
Copy link
Collaborator

@jkosternl jkosternl left a comment

Choose a reason for hiding this comment

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

Looking good, from a Java point of view.
If you're not on a tight schedule on CS, adding an unit test to verify the second prompt is correct, helps securing this.

@jkosternl
Copy link
Collaborator

Hi @palatsangeetha, if you're still coding in CS, can you also check this line in CsAction class?
if (!dsp.equals(updatedDsp)) dsp.set(updatedDsp); because that equals method is comparing different types of objects, and therefore is always not equal. You can also improve that later, if you don't like it now.

@palatsangeetha palatsangeetha merged commit ce2b8b9 into dev Sep 24, 2024
1 check passed
@palatsangeetha palatsangeetha deleted the APCV-1367-Incorrect-prompt-cs-conformance-issue branch September 24, 2024 08:56
@palatsangeetha
Copy link
Collaborator Author

Hi @palatsangeetha, if you're still coding in CS, can you also check this line in CsAction class? if (!dsp.equals(updatedDsp)) dsp.set(updatedDsp); because that equals method is comparing different types of objects, and therefore is always not equal. You can also improve that later, if you don't like it now.

@jkosternl Thanks for the comments on the CS PR, I will take care of it as part of a next change as changing that and then testing all flows might take some time.

@douradofrazer
Copy link

@CodiumAI-Agent /improve

@CodiumAI-Agent
Copy link

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Possible bug
Add null checks to prevent runtime exceptions

Consider checking for null values in sspSupplier.get() before calling toJson() to
prevent potential NullPointerExceptions.

commercial-schedules/src/main/java/org/dcsa/conformance/standards/cs/action/CsGetRoutingsAction.java [42-45]

 return previousAction instanceof CsGetRoutingsAction
     ? "Send a GET point to point routings request to fetch the second results page, using the cursor retrieved from the headers of the response of the first GET request."
     : "Send a GET point to point routings request with the following parameters: "
-        + sspSupplier.get().toJson().toPrettyString();
+        + (sspSupplier.get() != null ? sspSupplier.get().toJson().toPrettyString() : "No parameters available");
Suggestion importance[1-10]: 8

Why: Adding a null check for sspSupplier.get() is a practical suggestion to prevent potential NullPointerExceptions, which could lead to runtime errors. This enhances the robustness of the code.

8
Maintainability
Refactor to reduce code duplication and enhance maintainability

Refactor the constructor to avoid duplication in the ternary operation by extracting
the logic into a private method.

commercial-schedules/src/main/java/org/dcsa/conformance/standards/cs/action/CsGetPortSchedulesAction.java [19-26]

 super(
     subscriberPartyName,
     publisherPartyName,
     previousAction,
-    (previousAction instanceof CsGetPortSchedulesAction)
-        ? "GetPortSchedules (second page)"
-        : "GetPortSchedules",
+    determineActionLabel(previousAction),
     200);
Suggestion importance[1-10]: 7

Why: The suggestion to refactor the constructor by extracting the ternary operation into a private method is valid. It enhances code maintainability by reducing duplication and improving readability.

7
Enhancement
Improve code reuse and clarity by abstracting repetitive conditional logic into a method

Optimize the conditional logic by creating a method to handle the repeated
conditional checks across different actions.

commercial-schedules/src/main/java/org/dcsa/conformance/standards/cs/action/CsGetVesselSchedulesAction.java [23-30]

 super(
     subscriberPartyName,
     publisherPartyName,
     previousAction,
-    (previousAction instanceof CsGetVesselSchedulesAction)
-        ? "GetVesselSchedules (second page)"
-        : "GetVesselSchedules",
+    getActionLabel(previousAction, CsGetVesselSchedulesAction.class, "GetVesselSchedules"),
     200);
Suggestion importance[1-10]: 7

Why: The suggestion to abstract repetitive conditional logic into a method improves code clarity and reuse. It is a beneficial enhancement for maintaining consistency across similar actions.

7

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.

5 participants