Adal Chiriliuc wrote:
Me and my colleagues are having an discussion about the best way to
code a function (more Pythonic).
Here is the offending function:
def find(field, order):
....if not isinstance(order, bool):
........raise ValueError("order must be a bool")
....order_by = "asc" if order else "desc"
....return _find(field + "+" + order_by)
My opinions:
1. 'order' should be 'a' (the default) or 'd'. True and False by
themselves are meaningless. Then the check, if needed, is "if order not
in 'ad':".
'up' and 'down' are possible too.
2. Consider renaming _find as find, with two params and do the parameter
check there. Except when one is calling a function recursively
(repeatedly) with known-good params, wrapping a function with a simple
arg check seems like noise to me.
We are not sure what's the best practice here. Should we or should we
not check the type of the "order" variable, which should be a bool?
I say it should not be a bool. The below illustrates another reason why
this is the wrong type.
In one of our unit-tests we passed the "invalid_order" string as the
order argument value. No exception was raised, since the string was
evaluated as being True.
If you make 'order' a string, then a bad string input should raise an
exception somewhere. I suspect _find could be written to do this if it
does not already.
We know about "Don't look before we jump", but we are not sure how it
applies in this case, since we don't get any exception when passing an
invalid type argument.
This function is not visible to our clients, only internally in our
project. It's part of the public interface of a sub-system, so we are
not sure either if the fact that it returns an invalid result for a
badly type argument it's an API specification break or not.
The pro argument was that if a new programmer comes and passes a
wrongly typed argument, he will get a silent failure.
That is bad, but in this case, I see the problem as a slight mis-design.
The cons argument was that there are many functions which could
silently fail in this mode, especially those having bool arguments.
Which are rather rare, I think. In the 3.0 builtin functions, sorted's
'reverse' param is the only one. For back compatibility, it actually
accepts ints.
Should we in general check when doing unit-testing that the methods
behave correctly when passed arguments of the wrong type?
As in 'raise an exception', I think at least one test is good.
Terry Jan Reedy
--
http://mail.python.org/mailman/listinfo/python-list
