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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
