Needs more IllegalArgumentException



  • 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");
    

  • Discourse touched me in a no-no place

    @grkvlt said:

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

    FTFY


  • Winner of the 2016 Presidential Election

    Is it a feature that it can only return null?



  • 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.



  • @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...



  • @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?



  •  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).



  • @Medinoc said:

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

     

     even better... thanks!

     


Log in to reply
 

Looks like your connection to What the Daily WTF? was lost, please wait while we try to reconnect.