Re: [PR] Drop the symbol separator in the currency and percent re-parse [commons-validator]

["Copilot (via GitHub)" <gi...@apache.org>]

Copilot commented on code in PR #430:
URL: https://github.com/apache/commons-validator/pull/430#discussion_r3665152236


##########
src/main/java/org/apache/commons/validator/routines/PercentValidator.java:
##########
@@ -64,6 +64,18 @@ public static BigDecimalValidator getInstance() {
         return VALIDATOR;
     }
 
+    /**
+     * Tests whether the character at the given position of a pattern sits next to the percent symbol.
+     *
+     * @param pattern The pattern being stripped of its percent symbol.
+     * @param index The position to test.
+     * @return {@code true} if the character borders the percent symbol.
+     */
+    private static boolean isNextToSymbol(final String pattern, final int index) {
+        return index > 0 && pattern.charAt(index - 1) == PERCENT_SYMBOL
+                || index + 1 < pattern.length() && pattern.charAt(index + 1) == PERCENT_SYMBOL;
+    }

Review Comment:
   The boolean expression relies on `&&`/`||` precedence, which is easy to misread. Add explicit parentheses around the two adjacency checks to make intent unambiguous and reduce the risk of future edits changing behavior unintentionally.



##########
src/main/java/org/apache/commons/validator/routines/CurrencyValidator.java:
##########
@@ -59,6 +59,18 @@ public static BigDecimalValidator getInstance() {
         return VALIDATOR;
     }
 
+    /**
+     * Tests whether the character at the given position of a pattern sits next to the currency symbol.
+     *
+     * @param pattern The pattern being stripped of its currency symbol.
+     * @param index The position to test.
+     * @return {@code true} if the character borders the currency symbol.
+     */
+    private static boolean isNextToSymbol(final String pattern, final int index) {
+        return index > 0 && pattern.charAt(index - 1) == CURRENCY_SYMBOL
+                || index + 1 < pattern.length() && pattern.charAt(index + 1) == CURRENCY_SYMBOL;
+    }

Review Comment:
   Same as in `PercentValidator`: add parentheses to make the adjacency logic explicit and prevent precedence-related misunderstandings during maintenance.



##########
src/main/java/org/apache/commons/validator/routines/CurrencyValidator.java:
##########
@@ -59,6 +59,18 @@ public static BigDecimalValidator getInstance() {
         return VALIDATOR;
     }
 
+    /**
+     * Tests whether the character at the given position of a pattern sits next to the currency symbol.
+     *
+     * @param pattern The pattern being stripped of its currency symbol.
+     * @param index The position to test.
+     * @return {@code true} if the character borders the currency symbol.
+     */
+    private static boolean isNextToSymbol(final String pattern, final int index) {
+        return index > 0 && pattern.charAt(index - 1) == CURRENCY_SYMBOL
+                || index + 1 < pattern.length() && pattern.charAt(index + 1) == CURRENCY_SYMBOL;
+    }

Review Comment:
   The new helper method is duplicated in both `CurrencyValidator` and `PercentValidator` with only the symbol differing. Consider extracting a shared helper (e.g., `isNextToSymbol(pattern, index, symbolChar)`) to reduce duplication and keep the two implementations from drifting.



##########
src/test/java/org/apache/commons/validator/routines/PercentValidatorTest.java:
##########
@@ -100,6 +105,21 @@ void testNumberRangeExactBound() {
         assertFalse(instance.minValue(new BigDecimal("5"), new BigDecimal("5.5")));
     }
 
+    /**
+     * Test percentage values with a pattern that suffixes the symbol, which locales such as fr-FR separate from the number with a non-breaking space. The
+     * symbol is optional, so its separator has to be optional too.
+     */
+    @Test
+    void testSuffixSymbolPattern() {
+        final BigDecimalValidator validator = PercentValidator.getInstance();

Review Comment:
   This local variable shadows the test class field `validator`, which can be confusing when reading/debugging the test file. Rename the local (e.g., `instance` or `percentValidator`) to avoid shadowing.



##########
src/test/java/org/apache/commons/validator/routines/CurrencyValidatorTest.java:
##########
@@ -176,6 +179,21 @@ void testPattern() {
         assertFalse(validator.isValid(ukPound + "1,234.567", pattern, Locale.US), "invalid symbol");
     }
 
+    /**
+     * Test currency values with a pattern that suffixes the symbol, which locales such as de-DE separate from the number with a non-breaking space. The symbol
+     * is optional, so its separator has to be optional too.
+     */
+    @Test
+    void testSuffixSymbolPattern() {
+        final BigDecimalValidator validator = CurrencyValidator.getInstance();

Review Comment:
   Same shadowing issue as in `PercentValidatorTest`: this local `validator` can be confused with other `validator` variables/fields in the test. Rename the local to avoid shadowing and improve readability.



-- 
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: issues-unsubscribe@commons.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org