Thursday, August 19, 2010

Can you see the problem with this code?

Just a little code quiz, can you spot the design problem with this API? Bonus points for come up with a plausible reason as to what the programmer was distracted with whilst working on this.

    /** Load an integer from system properties, validating in a range */
    public static int getProperty(String name, int defValue, int min, int max){
        try {
            int value = Integer.getInteger(name, defValue).intValue();
            return Math.max(min, Math.min(max, value));
        }
        catch(NumberFormatException e) {
            return defValue;
        }
    }
    /** Load an long from system properties, validating in a range */
    public static long getProperty(String name, long min, long max, long defValue){
        try {
            long value = Long.getLong(name, defValue).longValue();
            return Math.max(min, Math.min(max, value));
        }
        catch(NumberFormatException e) {
            return defValue;
        }
    }

6 comments:

bob said...

If a NumberFormatException occurs, no range validation is performed on defValue?

Rose and Chris said...

It's the inconsistent ordering of parameters between the two very similar methods, right? And was he distracted by reading blogs?

jambay said...

+1 for Rose and Chris

manish4u@gmail.com said...

As per the method description, the defValue should be returned in case the name is not a valid property or the range is violated. But the as per the code, it would return max or min value instead of defValue if the range is not satisfied.

Secondly, the try catch block for NumberFormatException is not needed.

Gerard Davison said...

Yes

"Rose and Chris" get the points on this one. Having the different ordering on the method parameters is of course a really bad thing.

Gerard

Gerard Davison said...

I also think that Manish is right that the catch doesn't appear to be required and that def is not validated. (Bob also noticed the latter)

You might want def not to be validated though if you want to know if the value is undefined and you want to set something like Integer.MAX_VALUE to signal that. Depends on the context.