[bisq-network/bisq] Validate phone numbers (#3134)

[battleofwizards]

battleofwizards commented on this pull request.

Big plus for the test cases and meaningful names!

> +    @Getter
+    private final String callingCode;
+    /**
+     * The normalized (digits only) representation of an international calling code.
+     */
+    private final String normalizedCallingCode;
+    /**
+     * Phone number in E.164 format.
+     */
+    @Nullable
+    @Getter
+    private String normalizedPhoneNumber;
+
+    // Hide no-arg constructor
+    private PhoneNumberValidator() {
+        this.isoCountryCode = null;

These attributes are guaranteed null. Assigning null is superflous.

> +
+import java.util.Map;
+
+import static java.util.Map.entry;
+
+final class CountryCallingCodes {
+    /**
+     * Immutable mapping of ISO 3166 alpha-2 country code to national dialing number.
+     * <p>
+     * In some regions, such as American Samoa ('AS'), there is only one area code, and
+     * it is included in the mapping as part of the calling code.
+     *
+     * @see {@link <a href="https://en.wikipedia.org/wiki/E.164">https://en.wikipedia.org/wiki/E.164</a>}
+     * @see {@link <a href="https://en.wikipedia.org/wiki/List_of_country_calling_codes">https://en.wikipedia.org/wiki/List_of_country_calling_codes</a>}
+     */
+    private final Map<String, String> callingCodeMap = Map.ofEntries(

This should be static unless there is specific reason against.

> +     */
+    @Nullable
+    @Getter
+    private String normalizedPhoneNumber;
+
+    // Hide no-arg constructor
+    private PhoneNumberValidator() {
+        this.isoCountryCode = null;
+        this.countryCallingCodes = null;
+        this.callingCode = null;
+        this.normalizedCallingCode = null;
+    }
+
+    public PhoneNumberValidator(String isoCountryCode) {
+        this.isoCountryCode = isoCountryCode;
+        this.countryCallingCodes = new CountryCallingCodes();

Why do we keep an instance here? The CountryCallingCodes is essentially a global immutable resource.

> +
+    public PhoneNumberValidator(String isoCountryCode) {
+        this.isoCountryCode = isoCountryCode;
+        this.countryCallingCodes = new CountryCallingCodes();
+        this.callingCode = countryCallingCodes.getCallingCode(isoCountryCode);
+        this.normalizedCallingCode = countryCallingCodes.getNormalizedCallingCode(isoCountryCode);
+    }
+
+
+    @Override
+    public ValidationResult validate(String input) {
+        normalizedPhoneNumber = null;
+        ValidationResult result = super.validate(input);
+        if (!result.isValid) {
+            return result;
+        }

I might be wrong here but it feels like the dominating convention in the project is to skip `{}` for single statement blocks.

> +    public PhoneNumberValidator(String isoCountryCode) {
+        this.isoCountryCode = isoCountryCode;
+        this.countryCallingCodes = new CountryCallingCodes();
+        this.callingCode = countryCallingCodes.getCallingCode(isoCountryCode);
+        this.normalizedCallingCode = countryCallingCodes.getNormalizedCallingCode(isoCountryCode);
+    }
+
+
+    @Override
+    public ValidationResult validate(String input) {
+        normalizedPhoneNumber = null;
+        ValidationResult result = super.validate(input);
+        if (!result.isValid) {
+            return result;
+        }
+        final String trimmedInput = input.trim();

A matter of personal taste but I suggest instead of making local variables `final` make your methods very tiny. Then there is no need for the `final` keyword boilerplate.

> +import static org.junit.Assert.assertTrue;
+
+public class PhoneNumberValidatorTest {
+    private PhoneNumberValidator validator;
+    private ValidationResult validationResult;
+
+    @Before
+    public void setup() {
+        Res.setup();
+    }
+
+    @Test
+    public void testNoInput() {
+        validator = new PhoneNumberValidator("AT");
+        validationResult = validator.validate("");
+        assertFalse("'' should not be a valid number in AT", validationResult.isValid);

It really helps with test readability to define custom assertions. In this case `assertInvalid`.

> +        this.callingCode = countryCallingCodes.getCallingCode(isoCountryCode);
+        this.normalizedCallingCode = countryCallingCodes.getNormalizedCallingCode(isoCountryCode);
+    }
+
+
+    @Override
+    public ValidationResult validate(String input) {
+        normalizedPhoneNumber = null;
+        ValidationResult result = super.validate(input);
+        if (!result.isValid) {
+            return result;
+        }
+        final String trimmedInput = input.trim();
+        boolean isCountryDialingCodeExplicit = trimmedInput.startsWith("+");
+        // remove non-digit chars:  plus sign, dashes, parens, hashes, slashes and white spaces
+        String pureNumber = input.replaceAll("[\\+\\s+()\\-\\−\\#\\\\]", "");

Instead of removing specific chars remove everything that is not a digit: `^\\d` (regexp not tested).

> +    public ValidationResult validate(String input) {
+        normalizedPhoneNumber = null;
+        ValidationResult result = super.validate(input);
+        if (!result.isValid) {
+            return result;
+        }
+        final String trimmedInput = input.trim();
+        boolean isCountryDialingCodeExplicit = trimmedInput.startsWith("+");
+        // remove non-digit chars:  plus sign, dashes, parens, hashes, slashes and white spaces
+        String pureNumber = input.replaceAll("[\\+\\s+()\\-\\−\\#\\\\]", "");
+        boolean hasValidCallingCodePrefix = pureNumber.startsWith(normalizedCallingCode);
+        boolean isCountryCallingCodeImplicit = !isCountryDialingCodeExplicit && hasValidCallingCodePrefix;
+
+        result = validateIsNumeric(input, pureNumber)
+                .and(validateHasSufficientDigits(input, pureNumber))
+                .and(validateIsTooLong(input, pureNumber))

Maybe `validateIsNotTooLong`? Other validations are positive.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/bisq-network/bisq/pull/3134#pullrequestreview-279207090
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20190823/72ffda32/attachment.html>