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:
If a NumberFormatException occurs, no range validation is performed on defValue?
It's the inconsistent ordering of parameters between the two very similar methods, right? And was he distracted by reading blogs?
+1 for Rose and Chris
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.
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
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.
Post a Comment