Pros and cons of forbidding an empty string argument and throwing a RuntimeException in order to prevent the method from proceeding

Question

Say I have the following method definition in a Spring service layer:

@Override
public boolean passwordsMatch(String encrypted, String plain) {
      if (encrypted == null || encrypted.isEmpty() || plain == null || plain.isEmpty()) {
            throw new IllegalArgumentException("One argument is null or empty");
        }
      return passwordEncoder.matches(plain, encrypted);
   }

It is called by a Spring MVC application controller that wants to check whether the new password provided by a user (here called "plain") matches the user's current password (here called "encrypted").

Should I really be throwing an IllegalArgumentException (or any subtype of RuntimeException) if the new password provided by the user is empty (plain.isEmpty())?

I am 95% sure I am going to remove this check but I'd be curious to hear arguments in favor of keeping the check in this particular case.

Answer 1

IllegalArgumentException should be your first choice whenever you get an argument that you don't like within a method (that's the very purpose of this exception). So, unless you already have (or feel the need to implement) a more specific / meaningful exception, I think it's OK to stick with IllegalArgument/IllegalState for the sake of consistency.

However, it may be a good idea to point out the specific argument that you don't like in the exception message. And by the way, Guava provides very nice support for such validations with its Preconditions utility.

Preconditions.checkArgument(encrypted != null && !encrypted.isEmpty(), "The old password hash is empty");
Preconditions.checkArgument(plain != null && !plain.isEmpty(), "The new password is empty");

---

Now that you've clarified the actual question scope, I would say that you're the only one to decide whether your method should proceed if provided with null or empty arguments, after figuring out what exactly this method is supposed to check.

Based on the name of your method, I would say that it should at most disallow null values, as empty passwords can still be matched against their encrypted representation. The "minimum password length" rule should most likely be implemented elsewhere; this method should only report whether a plain password matches a hash, disregarding whether it's a legal password or not.

Answer 2

I wrote too fast. The method seems to be some kind of service method for comparing passwords. The domain is irrelevant. In this specific case it makes sense to throw an IllegalArgumentException if the provided arguments are null for example, but not for empty Strings as that might actually be a password.

---

In a use case that involves domain validation, I wouldn't go with IllegalArgumentException. Your User instance (or whatever) would not be in a valid state if its password was empty. You should therefore throw some kind of InvalidDomainException, ex. InvalidPasswordException (or use a BindingResult with a Validator).