Needs more IllegalArgumentException

grkvlt1

I found this in some validation code. It looks like there has been a failed attempt to follow the pattern that was used for all the other validation methods, coupled with a misunderstanding about what exceptions are for:

    public String validateThing(@Nullable String thing, String objectName, String fieldName) {
        if (thing != null) { // thing can be null.
            try {
                if (!thing.equalsIgnoreCase(Thing.CONSTANT))
                    throw new IllegalArgumentException();
            } catch (IllegalArgumentException e) {
                throw new IllegalArgumentException(messages.getMessage("validation.thing", new Object[]]{objectName, fieldName}, locale.get()));
            }
        }
        return null;
    }

The return null at the end is particularly fun, since these methods are meant to be used like this:

    String thing = validateThing(input, "object", "field");

antiquarian

@grkvlt said:

I found this in some validation code. It looks like there has been a failed coder.

FTFY

error

Is it a feature that it can only return null?

leonardo_

actually: yes! This method should have a void return type, and should simply throw the exception or complete normally. A method named "validateFoo" should not return a value.

String validatedFoo = validateFoo(foo);//why reassign your output into your input? was the original value good or did it change??

Now, if your method were named "cleanseFoo", or "formatFoo", then i'd expect to see you use it like:

String cleanFoo = cleanseFoo(foo); //the name of this method accurately reflects what it's doing.

ekolis

@_leonardo_ said:

actually: yes! This method should have a void return type, and should simply throw the exception or complete normally. A method named "validateFoo" should not return a value.

It could return a value, if the value is an error message or list of error messages to display to the user. I've actually done this a few times; not sure if it's a bad idea, but it got the job done! I'm guessing OP's code used to return an error message, but someone told whoever wrote it to make it throw exceptions instead, so he did so, but never actually changed the return type...

grkvlt1

@ekolis said:

I'm guessing OP's code used to return an error message

Actually, no. The code here was originally meant to be called similarly to the Guava library Preconditions.checkNotNull method. So, something like this:

String checked = Preconditions.checkNotNull(argv[0], "the input was null"); // throws NullPointerException
String thing = validator.validateThing(argv[1], "thing", "thingy"); // throws IllegalArgumentException

But, the other Guava precondition check methods in that class don't return their argument, and in the code that calls our validators the return value is always discarded. So, I think I should just make them void return types, like leonardo suggests...

My WTF, perhaps?

Medinoc

I'm not sure, more than half of the methods in the page you linked do return their argument (probably because it allows putting them directly in a method call, e. g. obj.Method(checkNotNull(arg))).

I guess it's just a matter of convenience, and code conciseness (putting validation on separate lines is cumbersome).

leonardo_

@Medinoc said:

...obj.Method(checkNotNull(arg)))....

even better... thanks!