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

SD-589 Handle exception and add unit test cases #238

Merged
merged 1 commit into from
Dec 6, 2024
Merged

SD-589 Handle exception and add unit test cases #238

merged 1 commit into from
Dec 6, 2024

Conversation

preetamnpr
Copy link
Collaborator

@preetamnpr preetamnpr commented Dec 5, 2024

PR Type

enhancement, tests


Description

  • Introduced fromString methods in BookingState and BookingCancellationState for safer parsing of state strings, replacing the use of valueOf.
  • Enhanced error handling by throwing IllegalArgumentException for unknown states.
  • Added comprehensive unit tests for the new fromString methods to ensure correct functionality and error handling.

Changes walkthrough 📝

Relevant files
Enhancement
BookingChecks.java
Use safer state parsing methods in BookingChecks                 

booking/src/main/java/org/dcsa/conformance/standards/booking/checks/BookingChecks.java

  • Replaced BookingState.valueOf with BookingState.fromString for safer
    state parsing.
  • Replaced BookingCancellationState.valueOf with
    BookingCancellationState.fromString.
  • +6/-6     
    PersistableCarrierBooking.java
    Enhance state parsing in PersistableCarrierBooking             

    booking/src/main/java/org/dcsa/conformance/standards/booking/model/PersistableCarrierBooking.java

    • Replaced valueOf with fromString for parsing booking states.
    +3/-3     
    BookingCancellationState.java
    Add fromString method to BookingCancellationState               

    booking/src/main/java/org/dcsa/conformance/standards/booking/party/BookingCancellationState.java

  • Added fromString method for safer parsing of cancellation states.
  • +9/-1     
    BookingState.java
    Add fromString method to BookingState                                       

    booking/src/main/java/org/dcsa/conformance/standards/booking/party/BookingState.java

    • Added fromString method for safer parsing of booking states.
    +10/-1   
    Tests
    BookingCancellationStateTest.java
    Add tests for BookingCancellationState fromString method 

    booking/src/test/java/org/dcsa/conformance/standards/booking/party/BookingCancellationStateTest.java

    • Added unit tests for BookingCancellationState.fromString.
    +39/-0   
    BookingStateTest.java
    Add tests for BookingState fromString method                         

    booking/src/test/java/org/dcsa/conformance/standards/booking/party/BookingStateTest.java

    • Added unit tests for BookingState.fromString.
    +28/-0   

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link

    qodo-merge-pro bot commented Dec 5, 2024

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Error Handling
    The error message in fromString() method could include the list of valid states to help users understand what values are allowed

    Test Coverage
    The test cases only check a subset of the valid booking states. Should test all possible enum values to ensure complete coverage

    Copy link

    qodo-merge-pro bot commented Dec 5, 2024

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Improve input validation with specific error messages for null/empty values

    Add input validation to check for empty/blank strings before attempting valueOf()
    conversion, to provide more specific error messages and avoid misleading "Unknown
    booking state" errors for empty inputs.

    booking/src/main/java/org/dcsa/conformance/standards/booking/party/BookingState.java [19-24]

     public static BookingState fromString(String bookingStateName) {
    +  if (bookingStateName == null || bookingStateName.isBlank()) {
    +    throw new IllegalArgumentException("Booking state cannot be null or empty");
    +  }
       try {
         return valueOf(bookingStateName); // Case-sensitive
    -  } catch (IllegalArgumentException | NullPointerException e) {
    +  } catch (IllegalArgumentException e) {
         throw new IllegalArgumentException("Unknown booking state: " + bookingStateName);
       }
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion improves error handling by providing more specific error messages for null/empty inputs and separates concerns in the validation logic, making the code more maintainable and user-friendly.

    8
    Standardize input validation across enum parsing methods for consistent error handling

    Add input validation to check for empty/blank strings before attempting valueOf()
    conversion, similar to BookingState, for consistent error handling across the
    codebase.

    booking/src/main/java/org/dcsa/conformance/standards/booking/party/BookingCancellationState.java [8-14]

     public static BookingCancellationState fromString(String bookingCancellationStateName) {
    +  if (bookingCancellationStateName == null || bookingCancellationStateName.isBlank()) {
    +    throw new IllegalArgumentException("Booking cancellation state cannot be null or empty");
    +  }
       try {
         return valueOf(bookingCancellationStateName);
    -  } catch (IllegalArgumentException | NullPointerException e) {
    +  } catch (IllegalArgumentException e) {
         throw new IllegalArgumentException("Unknown booking cancellation state: " + bookingCancellationStateName);
       }
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion ensures consistent error handling across the codebase by implementing the same validation pattern as in BookingState, which is crucial for maintaining code quality and predictable behavior.

    8
    Possible issue
    Add null check before parsing enum to prevent NullPointerException and provide clearer error messaging

    Add null check before calling fromString() to avoid potential NullPointerException
    when bookingStatus is null. The current error message in fromString() mentions
    "Unknown booking state" which is confusing for null values.

    booking/src/main/java/org/dcsa/conformance/standards/booking/checks/BookingChecks.java [412]

    -if (PENDING_CHANGES_STATES.contains(BookingState.fromString(bookingStatus))) {
    +if (bookingStatus != null && PENDING_CHANGES_STATES.contains(BookingState.fromString(bookingStatus))) {
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding a null check before enum parsing would prevent potential runtime exceptions and provide clearer error messages. This is important for robust error handling in a booking system.

    7

    💡 Need additional feedback ? start a PR chat

    @jkosternl jkosternl changed the title SD-589 handled exception and added unit test cases. SD-589 Handle exception and add unit test cases Dec 5, 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 to me. One minor remark written down. I also changed the grammar of the title to active voice. Commit messages are never in the past tense.

    @Test
    void testFromString_caseInsensitiveInput() {
    assertThrowsExactly(IllegalArgumentException.class, () -> BookingState.fromString("start"));
    assertThrowsExactly(IllegalArgumentException.class, () -> BookingState.fromString("received"));
    Copy link
    Collaborator

    Choose a reason for hiding this comment

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

    Please use assertThrows if that also fixes the job.
    In BookingCancellationStateTest.java you did that as well, which is 👍🏻

    Copy link
    Collaborator

    @palatsangeetha palatsangeetha left a comment

    Choose a reason for hiding this comment

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

    Looks good to me.

    @preetamnpr preetamnpr merged commit 1ef13f3 into dev Dec 6, 2024
    1 check passed
    @preetamnpr preetamnpr deleted the SD-589 branch December 6, 2024 10:03
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    3 participants