Potential WTF?



  • Is this a WTF or just over-coding? Context: Parsing configuration files.

    private static final String[] TRUE_VALUES = { "yes", "y", "true", "t", "affirm" };
    private static final String[] FALSE_VALUES = { "no", "n", "false", "f" };

    public static final boolean toBoolean(String value) {
    if (value == null) {
    throw new IllegalArgumentException("input cannot be null");
    }

    String tempValue = value.toLowerCase();

    boolean isTrueValue = false;
    for (String test : TRUE_VALUES) {
    if (test.equals(tempValue)) {
    isTrueValue = true;
    break; // out of loop
    }
    }

    if (!isTrueValue) {
    isTrueValue = true;
    for (String test : FALSE_VALUES) {
    if (test.equals(tempValue)) {
    isTrueValue = false;
    break; // out of loop
    }
    }
    } else {
    return isTrueValue;
    }

    if (isTrueValue) {
    throw new IllegalArgumentException("value must be one of the accepted values");
    }

    return isTrueValue;
    }



  • Not real sure here.

    It looks like there are defined vaues the system sees as true, perhaps 1, true, yes, checked and it looks at these values for true values.

    But then it checks a defined list of false values, perhaps 0, false, no, unchecked, and returns a false value for these.

    It is basically converting system defined values to true booleans, but at the end if the passed in value is not one of the system defined values for either true or false, it will throw an exception because it can not be considered either.

    Seems like safe programming here when parsing data here.

    Keep in mind this is not programmatic true or false, but rather data, that could possibly come from a third party they are parsing, hard to tell from this little bit of code.


  • Discourse touched me in a no-no place

    @zelmak said:

    private static final String[ TRUE_VALUES = { "yes", "y", "true", "t", "affirm" };

    private static final String[ FALSE_VALUES = { "no", "n", "false", "f" };
    If they're accepting 'affirm' why aren't they accepting 'deny' (or 'negate', or 'veto' or...)? And "one" and "zero" are also missing.



  • Just over-coding.


    Not a WTF in sight.


    Not a single one.



  • @Hatshepsut said:

    Just over-coding.

    Not a WTF in sight.

    Not a single one.

    I think there could be one, but as I said before hare to tell.

    If this is part of parsing third party data, which you have no control over the format then this looks safe, and really not over coded at all.

    But, if this is in the translation of data already in your system, I would have to ask why isn't the data standardized within your repository already?  Save the boolean true false, but display the appropriate value.  This is where you draw the line between data and view.  If you are placing view values in the database instead of the boolean, I would call that a WTF, which this code would be an artifact of.



  • @KattMan said:

    @Hatshepsut said:

    Just over-coding.

    Not a WTF in sight.

    Not a single one.

    I think there could be one, but as I said before hare to tell.

    If this is part of parsing third party data, which you have no control over the format then this looks safe, and really not over coded at all.

    But, if this is in the translation of data already in your system, I would have to ask why isn't the data standardized within your repository already?  Save the boolean true false, but display the appropriate value.  This is where you draw the line between data and view.  If you are placing view values in the database instead of the boolean, I would call that a WTF, which this code would be an artifact of.

    Only the third party type. Yes, we have stuff doing translation between visual/view and logical/stored ... and they are some real humdingers ... but this isn't in that part. Solely for parsing stupid-user inputted data.



  • @PJH said:

    @zelmak said:
    private static final String[ TRUE_VALUES = { "yes", "y", "true", "t", "affirm" };
    private static final String[ FALSE_VALUES = { "no", "n", "false", "f" };
    If they're accepting 'affirm' why aren't they accepting 'deny' (or 'negate', or 'veto' or...)? And "one" and "zero" are also missing.

    🤷



  • @zelmak said:

    Only the third party type. Yes, we have stuff doing translation between visual/view and logical/stored ... and they are some real humdingers ... but this isn't in that part. Solely for parsing stupid-user inputted data.

    Then I would say no, this is not a WTF, it's good practice for a translation to get stadardized data into your system.



  • Coding is a bit lousy. Instead of breaking out of the loop, you could return true or false. And of course, you could use ArrayList and then contains:

    if (TRUE_VALUES.contains(tempValue)) return true;

    if (FALSE_VALUES.contains(tempValue)) return false;

    throw new IllegalArgumentException("value must be one of the accepted values");



  • @TGV said:

    Coding is a bit lousy. Instead of breaking out of the loop, you could return true or false. And of course, you could use ArrayList and then contains:

    if (TRUE_VALUES.contains(tempValue)) return true;

    if (FALSE_VALUES.contains(tempValue)) return false;

    throw new IllegalArgumentException("value must be one of the accepted values");

    Well, obviously. But I'm wondering what Hatshepsut has seen, if he's being sarcastic rather than pointlessly haikuic.



  • As pointed out above, using an ArrayList.contains() might be less wordy, but sanitizing input from multiple sources not under your control always looks something like this. I've had to do it myself.

    Definitely not a WTF.

    Be thankful that the data is sanitized in one place instead of having the same checks everyplace it's referenced (I've had to clean up that mess a couple of times).

     



  • The structure is horrifying, but as has been said, the idea behind it isn't bad. I'd personally merge both TRUE_VALUES and FALSE_VALUES into one map/dictionary, then just check if the input string is even in there at all.



  • @Salamander said:

    The structure is horrifying, but as has been said, the idea behind it isn't bad. I'd personally merge both TRUE_VALUES and FALSE_VALUES into one map/dictionary, then just check if the input string is even in there at all.

    That changes the purpose.  It is not just seeing if there is a valid value, but also translating it to a real boolean.  So values that are considered true become true and values considered false become false.



  • @KattMan said:

    @Salamander said:

    The structure is horrifying, but as has been said, the idea behind it isn't bad. I'd personally merge both TRUE_VALUES and FALSE_VALUES into one map/dictionary, then just check if the input string is even in there at all.

    That changes the purpose.  It is not just seeing if there is a valid value, but also translating it to a real boolean.  So values that are considered true become true and values considered false become false.

     

    Yes, but a map/dictionary stores things as key/value pairs.  So I'm assuming that Salamander meant that each string would be a key and the associated value would be the boolean translation of it.

     



  • @DescentJS said:

    @KattMan said:

    @Salamander said:

    The structure is horrifying, but as has been said, the idea behind it isn't bad. I'd personally merge both TRUE_VALUES and FALSE_VALUES into one map/dictionary, then just check if the input string is even in there at all.

    That changes the purpose.  It is not just seeing if there is a valid value, but also translating it to a real boolean.  So values that are considered true become true and values considered false become false.

     

    Yes, but a map/dictionary stores things as key/value pairs.  So I'm assuming that Salamander meant that each string would be a key and the associated value would be the boolean translation of it.

     

    Come to think of it that would work well, create the dictionary, check to see if it even exists and return the value mapped, no need to check separate values.  Didn't think of it that way.

    Whoa wait a mnute, we can't have that, this is the Daily WTF, nothing sane can be posited and accepted here!

     

     


  • BINNED

    @token_woman said:

    But I'm wondering what Hatshepsut has seen, if he's being sarcastic rather than pointlessly haikuic.
    I assumed he was being both.



  • @TGV said:

    Coding is a bit lousy. Instead of breaking out of the loop, you could return true or false. And of course, you could use ArrayList and then contains:

    if (TRUE_VALUES.contains(tempValue)) return true;

    if (FALSE_VALUES.contains(tempValue)) return false;

    throw new IllegalArgumentException("value must be one of the accepted values");



    I do not like this. Exceptions are used to indicate an exception to normal code flow, and should not be used as an integral part of validation or any other logic.

    I like the dictionary idea, use a TryGetValue. The return value indicates if the the input is valid and the out param the correlation.


  • @Hatshepsut said:

    Just over-coding.


    Not a WTF in sight.


    Not a single one.

    I'd put it another way: the code stinks, in that it's a chore to determine whether or not it does the right thing. Not a huge problem if you can just call it and trust the results, but a pain in the ass if, e.g., management decides that it's time to add some different strings, perhaps even support different locales.



  • @this_code_sucks said:

    @TGV said:

    Coding is a bit lousy. Instead of breaking out of the loop, you could return true or false. And of course, you could use ArrayList and then contains:

    if (TRUE_VALUES.contains(tempValue)) return true;

    if (FALSE_VALUES.contains(tempValue)) return false;

    throw new IllegalArgumentException("value must be one of the accepted values");



    I do not like this. Exceptions are used to indicate an exception to normal code flow, and should not be used as an integral part of validation or any other logic.

    I like the dictionary idea, use a TryGetValue. The return value indicates if the the input is valid and the out param the correlation.

    So, what value would you return if the user called it with the value of "Albequerque"?

    FILE_NOT_FOUND?



  • @zelmak said:

    @this_code_sucks said:

    @TGV said:

    Coding is a bit lousy. Instead of breaking out of the loop, you could return true or false. And of course, you could use ArrayList and then contains:

    if (TRUE_VALUES.contains(tempValue)) return true;

    if (FALSE_VALUES.contains(tempValue)) return false;

    throw new IllegalArgumentException("value must be one of the accepted values");



    I do not like this. Exceptions are used to indicate an exception to normal code flow, and should not be used as an integral part of validation or any other logic.

    I like the dictionary idea, use a TryGetValue. The return value indicates if the the input is valid and the out param the correlation.

    So, what value would you return if the user called it with the value of "Albequerque"?

    FILE_NOT_FOUND?




    They are two seperate functions:


    Dictionary<string, bool> _binaryResponses = stuff;


    bool IsValidResponse(string r)

    {

    return _binaryResponses.Cotains(r);
    }


    Also you could use an out param or a tuple, although not what I would do. I'm not saying you should not throw an exception, just that it should not be a normal part of the code logic and it should only be triggered by programmer forgetting to call IsValidResponse before calling the translator function.



  • Sorry to hate on your C# love-fest, but the OP is clearly written in Java which has neither tuples or out parameters. Throwing an exception is the only sane way to indicate the value is neither true or false.


    Well, you could return a null Boolean instead, but throwing an exception is more appropriate.



  • @this_code_sucks said:

    They are two seperate functions:

    Dictionary<string, bool> _binaryResponses = stuff;

    bool IsValidResponse(string r)

    {

    return _binaryResponses.Cotains(r);
    }


    Also you could use an out param or a tuple, although not what I would do. I'm not saying you should not throw an exception, just that it should not be a normal part of the code logic and it should only be triggered by programmer forgetting to call IsValidResponse before calling the translator function.

    Okay, then what action is taken when isValidResponse() returns false? Again, I'm talking about a config file here for server-side processes. There's not going to be an alert window telling the user "Hey, sorry old boy, you've screwed the pooch on that configuration item." Seems to me, throwing an exception for exceptional circumstances -- an invalid entry in the configuration file -- is quite valid here. I suppose if you have sane defaults, you could just log the error and press on, but if that item is critical to the proper operation of your code, throwing an exception and crashing out is completely valid.

    While I might munge it up a bit to include the 'dictionary contains' bits, changing the exception behavior might have other effects. You have to remember what a fragile state this rotting cesspool of a code base is in ... change one thing and the entire system comes falling to its knees...



  • @PJH said:

    @zelmak said:
    private static final String[ TRUE_VALUES = { "yes", "y", "true", "t", "affirm" };
    private static final String[ FALSE_VALUES = { "no", "n", "false", "f" };
    If they're accepting 'affirm' why aren't they accepting 'deny' (or 'negate', or 'veto' or...)? And "one" and "zero" are also missing.
     

    and 1 and 0, yup/nope, yarr/narr, on/off, enabled/disabled, active/inactive...

    ...also, can we have "ahoy, ye scurvy dogs" as a deployment config section delimiter and "ye bloody drylanders" for dev config section?*

     

     

    *or something along those lines, I probably overestimated my ability to speak pirate when I was starting that sentence



  •  The code is messy and has several boo-boos, but a real WTF?

    • The fact that it checks for null and throws an unchecked exception (IllegalArgumentException) is a bit silly, because NullPointerException is an unchecked exception, too.
    • The author is  apparerntly unfamiliar with the String.equalsIgnoreCase method. Or perhaps this is some attempt at optimisation, but with such inefficient code, that would be a moot point.

    Functionally equivalent, and a lot easier to read:

    private final static Collection<String> TRUE_VALUES = Arrays.asList( "yes", "y", "true", "t", "affirm" );
    private final static Collection<String> FALSE_VALUES = Arrays.asList( "no", "n", "false", "f" );

    public final static boolean toBoolean( String value )
    {
        if( TRUE_VALUES.contains( value.toLowerCase() )
            return true;

        if( FALSE_VALUES.contains( value.toLowerCase() )
            return false;

        throw new IllegalArgumentException( "value must be one of the accepted values" );
    }

     



  • @Severity One said:

     The code is messy and has several boo-boos, but a real WTF?

    • The fact that it checks for null and throws an unchecked exception (IllegalArgumentException) is a bit silly, because NullPointerException is an unchecked exception, too.
    • The author is  apparerntly unfamiliar with the String.equalsIgnoreCase method. Or perhaps this is some attempt at optimisation, but with such inefficient code, that would be a moot point.

    Functionally equivalent, and a lot easier to read:

    private final static Collection<String> TRUE_VALUES = Arrays.asList( "yes", "y", "true", "t", "affirm" );
    private final static Collection<String> FALSE_VALUES = Arrays.asList( "no", "n", "false", "f" );

    public final static boolean toBoolean( String value )
    {
        if( TRUE_VALUES.contains( value.toLowerCase() )
            return true;

        if( FALSE_VALUES.contains( value.toLowerCase() )
            return false;

        throw new IllegalArgumentException( "value must be one of the accepted values" );
    }

     

    You're right - on reflection I think this is mostly just (very) horrible coding.


    The 'affirm' might be there for any number of reasons, some of which are not very WTFy, and some of which are.


    Which makes me wonder whether some kind of standard scale for WTFery should be established. (Actually, at least two would be required: metric and US/imperial.)



  • @zelmak said:

    @this_code_sucks said:

    They are two seperate functions:

    Dictionary<string, bool> _binaryResponses = stuff;

    bool IsValidResponse(string r)

    {

    return _binaryResponses.Cotains(r);
    }


    Also you could use an out param or a tuple, although not what I would do. I'm not saying you should not throw an exception, just that it should not be a normal part of the code logic and it should only be triggered by programmer forgetting to call IsValidResponse before calling the translator function.

    Okay, then what action is taken when isValidResponse() returns false? Again, I'm talking about a config file here for server-side processes. There's not going to be an alert window telling the user "Hey, sorry old boy, you've screwed the pooch on that configuration item." Seems to me, throwing an exception for exceptional circumstances -- an invalid entry in the configuration file -- is quite valid here. I suppose if you have sane defaults, you could just log the error and press on, but if that item is critical to the proper operation of your code, throwing an exception and crashing out is completely valid.

    While I might munge it up a bit to include the 'dictionary contains' bits, changing the exception behavior might have other effects. You have to remember what a fragile state this rotting cesspool of a code base is in ... change one thing and the entire system comes falling to its knees...



    Forgot that it was a config file.

    If it's a config file, why not only accept true or false?


  • @SEMI-HYBRID code said:

    @PJH said:

    @zelmak said:
    private static final String[ TRUE_VALUES = { "yes", "y", "true", "t", "affirm" };
    private static final String[ FALSE_VALUES = { "no", "n", "false", "f" };
    If they're accepting 'affirm' why aren't they accepting 'deny' (or 'negate', or 'veto' or...)? And "one" and "zero" are also missing.
     

    and 1 and 0, yup/nope, yarr/narr, on/off, enabled/disabled, active/inactive...

    ...also, can we have "ahoy, ye scurvy dogs" as a deployment config section delimiter and "ye bloody drylanders" for dev config section?

     

    My favorite command for restarting an application suite after applying some shutdown-requiring patches:  "give my creation...LIFE!"

     



  • @Severity One said:

    The fact that it checks for null and throws an unchecked exception (IllegalArgumentException) is a bit silly, because NullPointerException is an unchecked exception, too.
    There is a school of thought in Java programming that explicitly states that, when you get passed a null argument into a function for a parameter that may not be null, then you should catch that early and throw an IllegalArgumentException, because that is much more indicative of what is actually happening (if you know the parameter must not be null, then null is not a legal value; hence, illegal argument). NullPointerExceptions should be reserved for those cases where a reference is null unexpectedly (and you should always anticipate and ward against the possibility of invalid arguments being passed in to your methods, so these are clearly expected).

    Not that I necessarily subscribe to that kind of view, but I can see the logic behind it.


Log in to reply