ankursha1 commented on PR #10256: URL: https://github.com/apache/gravitino/pull/10256#issuecomment-4018882550
> **Code Review** > > The intent is correct — fail fast when `modelVersionPOs` is null or empty rather than getting an `IndexOutOfBoundsException` on `.get(0)`. > > ### Issues > 1. **Prefer `isEmpty()`**: Use `!modelVersionPOs.isEmpty()` instead of `modelVersionPOs.size() > 0`. This is the idiomatic Java collection check and is recommended by the Google Java Style guide. > ```java > // Before > modelVersionPOs != null && modelVersionPOs.size() > 0 > // After > modelVersionPOs != null && !modelVersionPOs.isEmpty() > ``` > 2. **Missing null test case**: The test only exercises `Collections.emptyList()`. Please also add a test that passes `null` explicitly to cover the null branch of the precondition. > 3. **No conflict with #10292** — both modify `POConverters.java` but target different methods, so a clean merge should be possible. > > Please address items 1 and 2 before merging. @jerryshao Thanks for review. I have pushed the change to address the review comment. Please have a look. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
