ISBN validation with Hibernate Validator

Question

I've written a ISBN-validating annotation. It is to be used in conjunction with Hibernate Validator.

I'm pretty satisfied with this code as it works as expected, but I've had to make a design choice that I totally dislike. This is why I post this question.

The method checkISBN returns an Optional regarding the error returned. An empty Optional then means it's okay. Originally, I sincerely thought about using a custom exceptions, but exceptions shouldn't be used as code structure (Effective Java, 2nd edition, item 59). But the Optional used as a negative check totally bugs me. Plus, I feel like the method names are totally wrong, since I'm using Optional in an inverted way and the methods then don't say what they return.

Another point is that I usually favor static methods, but most of my check* methods aren't because of method references named ISBNvalidator::check* makes it feels like these methods aren't part of this class. Maybe that's just me.

In a lesser measure, please provide some advice on how to properly name my variables that start with ISBN. The classes will stay like that, but I just can't properly decide what to do regarding the variable names: use ISBNAnnotation or isbnAnnotation, for instance. Also, there's some inconsistency in what I decided: String isbn is named like that only because I didn't want to name a variable exactly the same as the annotation ISBN.

Other remarks are naturally welcome, but the previous points are more important in my eyes.

ISBN.java (The annotation created)

package myapp.book;
import static java.lang.annotation.ElementType.ANNOTATION_TYPE;
import static java.lang.annotation.ElementType.FIELD;
import static java.lang.annotation.ElementType.METHOD;
import static java.lang.annotation.ElementType.PARAMETER;
import static java.lang.annotation.RetentionPolicy.RUNTIME;
import java.lang.annotation.Documented;
import java.lang.annotation.Retention;
import java.lang.annotation.Target;
import javax.validation.Constraint;
@Target({FIELD, METHOD, PARAMETER, ANNOTATION_TYPE})
@Retention(RUNTIME)
@Constraint(validatedBy = ISBNValidator.class)
@Documented
public @interface ISBN {
 Format format() default Format.ANY;
 public enum Format {
 ISBN_10,
 ISBN_13,
 ANY;
 }
}

ISBNValidator.java

package myapp.book;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.CharMatcher;
import java.util.Optional;
import java.util.function.Predicate;
import java.util.regex.Pattern;
import javax.validation.ConstraintValidator;
import javax.validation.ConstraintValidatorContext;
public class ISBNValidator implements ConstraintValidator<ISBN, String> {
 private static final Predicate<String> ISBN_10_PATTERN = Pattern.compile("^[0-9]{9}[0-9X]$").asPredicate();
 private static final Predicate<String> ISBN_13_PATTERN = Pattern.compile("^[0-9]{13}$").asPredicate();
 private static final CharMatcher CLEANUP_MATCHER = CharMatcher.is('-');
 @VisibleForTesting
 static final String FORMAT_ERROR = "{books.ISBN.format}";
 @VisibleForTesting
 static final String CHECKSUM_ERROR = "{books.ISBN.checksum}";
 private ISBN.Format format;
 @Override
 public void initialize(ISBN ISBNAnnotation) {
 this.format = ISBNAnnotation.format();
 }
 @Override
 public boolean isValid(String isbn, ConstraintValidatorContext context) {
 if (isbn == null) {
 // That's not @ISBN's role, but @NotNull's
 return true;
 }
 isbn = CLEANUP_MATCHER.removeFrom(isbn);
 final Optional<String> error;
 switch (format) {
 case ISBN_10:
 error = checkISBN(isbn, ISBN_10_PATTERN, this::checksumISBN10);
 break;
 case ISBN_13:
 error = checkISBN(isbn, ISBN_13_PATTERN, this::checksumISBN13);
 break;
 case ANY:
 if (ISBN_10_PATTERN.test(isbn)) {
 error = checkISBN(isbn, x -> true, this::checksumISBN10);
 } else if (ISBN_13_PATTERN.test(isbn)) {
 error = checkISBN(isbn, x -> true, this::checksumISBN13);
 } else {
 error = Optional.of(FORMAT_ERROR);
 }
 break;
 default:
 throw new AssertionError("unreachable code");
 }
 error.ifPresent(e -> {
 context.disableDefaultConstraintViolation();
 context.buildConstraintViolationWithTemplate(e).addConstraintViolation();
 });
 return !error.isPresent();
 }
 private Optional<String> checkISBN(String isbn, Predicate<String> formatChecker, Predicate<String> sumChecker) {
 if (!formatChecker.test(isbn)) {
 return Optional.of(FORMAT_ERROR);
 }
 if (!sumChecker.test(isbn)) {
 return Optional.of(CHECKSUM_ERROR);
 }
 return Optional.empty();
 }
 private boolean checksumISBN10(String isbn) {
 int sum = 0;
 for (int i = 0; i < 9; i++) {
 sum += digit(isbn.charAt(i)) * (i + 1);
 }
 int computedChecksum = sum % 11;
 char cs = isbn.charAt(9);
 int checksum = cs == 'X' ? 10 : digit(cs);
 return checksum == computedChecksum;
 }
 private boolean checksumISBN13(String isbn) {
 int sum = 0;
 for (int i = 0; i < 12; i += 2) {
 sum += digit(isbn.charAt(i));
 sum += digit(isbn.charAt(i + 1)) * 3;
 }
 int computedChecksum = (10 - sum % 10) % 10;
 int checksum = digit(isbn.charAt(12));
 return checksum == computedChecksum;
 }
 private static int digit(char c) {
 return Character.digit(c, 10);
 }
}

Answer 1

I would not use an Optional here. Instead, I would have the method checkISBN receive the ConstraintValidatorContext as parameter, and let it populate the context explicitely.

Before you might say: "but we're modifying a parameter!", note that the intent of this context is to be modified. In fact, the purpose of the isValid method is to build constraint violations from this context. The signature of isValid, as defined by the Validation API, is:

boolean isValid(T value, ConstraintValidatorContext context);

As such, this method is intended to:

• return the boolean indicating whether validation passed or not;
• in case validation did not pass, build the violations with help of the given context.

Therefore, we can use this idea with the chechISBN method, and rename it to isISBNValid. Consider this:

private boolean isISBNValid(String isbn, Predicate<String> formatChecker, Predicate<String> sumChecker, ConstraintValidatorContext context) {
 if (!formatChecker.test(isbn)) {
 buildViolation(FORMAT_ERROR, context);
 return false;
 }
 if (!sumChecker.test(isbn)) {
 buildViolation(CHECKSUM_ERROR, context);
 return false;
 }
 return true;
}
private void buildViolation(String template, ConstraintValidatorContext context) {
 context.disableDefaultConstraintViolation();
 context.buildConstraintViolationWithTemplate(template).addConstraintViolation();
}

You can then use it with:

switch (format) {
case ISBN_10:
 return isISBNValid(isbn, ISBN_10_PATTERN, this::checksumISBN10, context);
case ISBN_13:
 return isISBNValid(isbn, ISBN_13_PATTERN, this::checksumISBN13, context);
case ANY:
 if (ISBN_10_PATTERN.test(isbn)) {
 return isISBNValid(isbn, x -> true, this::checksumISBN10, context);
 } else if (ISBN_13_PATTERN.test(isbn)) {
 return isISBNValid(isbn, x -> true, this::checksumISBN13, context);
 } else {
 buildViolation(FORMAT_ERROR, context);
 return false;
 }
default:
 throw new AssertionError("unreachable code");
}

---

Other comments:

• You're using Guava's CharMatcher to remove all occurences of the - character in the ISBN. You could also stick to the more simple:

  isbn = isbn.replace("-", "");
  

  Internally, this does create a literal Pattern which may have more overhead that the Guava version though.

• You could also make an enum of all the errors, instead of having them as constants.
• I don't think there is an accepted naming convention for acronyms in Java. I tend to use all uppercase for the classes (like your ISBN) and all lowercase for variables (like String isbn); this is just my personal preference though.

• Olivier Grégoire: Thanks! After so much refactoring, I couldn't think about proper stuff anymore. You've unlocked me. I just don't get how I could use the stream API in the methods you mentioned. Could you clarify that point?
• Tunaki: @OlivierGrégoire In fact, I removed that comment, after further thought, it won't make the code any clearer.

Answer 2

Why did you include Optional when it doesn't seem to be doing anything special?

To me, it seems you've just used Optional as a replacement for null checks. Which is what it is intended for, but it doesn't give you any real benefits.

I'd just do away with the Optional.

As for naming, IsbnValidator and String isbn and @Isbn is how you'd do it. This to prevent word soup like HTTPURLConnection.

• maaartinus: +1 for "word soup". The uppercasing natural language rules make acronyms like "ISBN" stand out nicely in natural text, but they're a readability and consistency disaster for Java.
• Tunaki: As a side-note, in the JDK, the class is in fact named HttpURLConnection, with uppercase for "short" (<=3 characters) acronyms but not for "long" (> 3 characters) acronyms. But the XML API has HTTPBinding contradicting this...
• Olivier Grégoire: @Pimgd "why...?" Because I started with an ugly solution, trying to inline everything and (ab)use the stream API. I kept refactoring until I got there and got stuck, hence the post.

Answer 3

Remove unnecessary dependencies

Try to minimize the "invasion" of additional libraries if you can simply say string.replace("-","").

Apply single responsibility principle

Extract code into additional classes:

ISBNSpecification

public interface ISBNSpecification {
 default boolean isISBNType(String potentialISBN) {
 return getPattern().matcher(potentialISBN).matches();
 }
 
 Pattern getPattern();
 boolean validateChecksum(String isbn);
 
}

ISBN13Specification

public class ISBN13Specification implements ISBNSpecification {
 @Override
 public Pattern getPattern() {
 return Pattern.compile("^[0-9]{13}$"); // you may extract this as a constant
 }
 @Override
 public boolean validateChecksum(String isbn) {
 int sum = 0;
 for (int i = 0; i < 12; i += 2) {
 sum += digit(isbn.charAt(i));
 sum += digit(isbn.charAt(i + 1)) * 3;
 }
 int computedChecksum = (10 - sum % 10) % 10;
 int checksum = digit(isbn.charAt(12));
 return checksum == computedChecksum;
 }
 private static int digit(char c) {
 return Character.digit(c, 10);
 }
}

ISBN10Specification

public class ISBN10Specification implements ISBNSpecification {
 @Override
 public Pattern getPattern() {
 return Pattern.compile("^[0-9]{9}[0-9X]$"); // you may extract this as a constant
 }
 @Override
 public boolean validateChecksum(String isbn) {
 int sum = 0;
 for (int i = 0; i < 9; i++) {
 sum += digit(isbn.charAt(i)) * (i + 1);
 }
 int computedChecksum = sum % 11;
 char cs = isbn.charAt(9);
 int checksum = cs == 'X' ? 10 : digit(cs);
 return checksum == computedChecksum;
 }
 private static int digit(char c) {
 return Character.digit(c, 10);
 }
 
}

• Olivier Grégoire: Why use one single class per "responsibility"? Is the class the new method? What's the gain when there are only two implementations like in my case? Regarding Guava, it's used all over the rest of the classes. So I won't remove the library itself. Since it's present, there's no reason to not use CharMatcher. If after usage, I notice that my users use other characters in their ISBN, will I chain replace() calls? Nope, I'll change my CharMatcher. I have it, I keep it.
• oopexpert: "Since it's present, there's no reason to not use CharMatcher." The problem is that this argument can be placed for EVERY dependency as long as it is present.
• oopexpert: "Why use one single class per responsibility?" I never did and the classes also have more than one method. I did not get the argument.
• oopexpert: "What's the gain when there are only two implementations like in my case." Abstraction (less if-then-else, separation of compilation units, smaller pieces to understand,
• Olivier Grégoire: @oopexpert I understand the theory, which you speak about. In this case, what's the gain?