Occasionally I’m going against some Enum options class like
from enum import Enum
class Options(Enum):
IDENTITY = 'identity'
DOUBLE = 'double'
SQUARE = 'square'
And I have some function that changes behavior depending on which option is passed:
def do_action(value: float, option: Options):
if option is Options.IDENTITY:
new_value = value
elif option is Options.DOUBLE:
new_value = value * 2
elif options is Options.SQUARE:
new_value = value**2
return new_value
But there is a problem with this code. My IDE (pycharm) often will complain on the return line that new_value may be referenced before it is assigned. This is because the static code checker doesn’t (can’t) know that my if ... elif ... elif ... pattern exhausts all possible options. I could instead do
def do_action(value: float, option: Options):
if option is Options.IDENTITY:
new_value = value
elif option is Options.DOUBLE:
new_value = value * 2
else:
new_value = value**2
return new_value
But this is less than ideal because it is preferable from a readability perspective to see Options.SQUARE. What I typically end up doing is something like
def do_action(value: float, option: Options):
if option is Options.IDENTITY:
new_value = value
elif option is Options.DOUBLE:
new_value = value * 2
elif options is Options.SQUARE:
new_value = value**2
else:
raise ValueError(f'Unhandled option {option}')
return new_value
Is this a good approach? Should I raise NotImplementedError? Or would this be an appropriate place for an assert? Or a totally different pattern?
This is a great approach, and up until python 3.10 I’d say that’s best practice.
If you are using 3.10 or higher, you could also use a match statement instead:
match option:
case Options.IDENTITY:
new_value = value
case Options.DOUBLE:
new_value = value * 2
case Options.SQUARE:
new_value = value**2
case _:
raise ValueError(f'Unhandled option {option}')
This has the same functionality, but might be easier to think about (or maybe not, up to you).
You can also just return right inside the conditional statement instead of assigning to new_value, which would silence your IDE’s complaints. But you might still want to catch the situation when option is something unexpected.
def calculate(left: int, op: Op, right: int) -> int:
if op is Op.ADD:
return left + right
elif op is Op.SUBTRACT:
return left - right
else:
assert_never(op)
(New in Python 3.11. Don’t know if pycharm knows about it, or what happens at runtime if you somehow reach that code anyway. Please test and report back here.)
This function has an inferred return type of float | None, because without exhaustiveness checking static analysis tools can’t know execution won’t fall off the end of the function block.
——————
Using a dispatch table (dict) is reasonable, and you could use TypedDict to make sure you’re including all the Enum cases.
——————-
Like @petersuter said, assert_never is designed for exactly this case - to coerce the type checker into knowing a set of conditions is exhaustive. The only thing I have to add is that if you don’t have 3.11 and still want to use it, I believe it’s also available in the typing_extensions package.
——————-
Personally I would opt for the match statement with inline returns like @jamestwebber mentioned, as I find it the clearest to read and maintain. (The match statement apparently has good performance speed as well, or so I’ve heard?) If PyCharm is capable of exhaustiveness checking then it may even type check without a trailing wildcard case. For additional runtime protection, add the last case regardless of the type checker and throw a ValueError like you’re doing.
———————
With or without static analysis tools, explicitly indicating the expected outcome when “the value should match one of these but somehow doesn’t match any of the things I expected” serves as documentation of your intent for others and your future self, besides detecting potential programming errors in a way that’s easier to debug. If you explicitly define the else case and throw an exception, then a mistake with the Enum parameter gets caught immediately and the exception tells you exactly what happened. Otherwise, the function returns None or a sentinel/default value that may work initially until execution has moved elsewhere and you start seeing subtly wrong results or an unexpected NoneType error that you have to trace through a chain of function calls.
I agree. HOWEVER! Adding unused code in case something changes in the future is a very good approach. There’s nothing inherent in these options that proves that there cannot possibly ever be any others, so I would say the “else raise” design makes a lot of sense.
If this shows up a lot, it might be worth having a helper function, but otherwise a direct raise statement would be fine.
I specifically referred to it as unused code because there’s absolutely no possibility for the parameter option to receive an argument of a type other than Options.
Yes, this is essentially why I asked the question. I agree it is unused code. That’s why I wondered if it was an appropriate place for an assert. Actually thinking about it more, this code is NOT unused. This code is used in the event that users pass incorrect input and the error alerts them of that. That also answers the assert question. This is exactly NOT the place to use the assert because users could access that code by passing bad input. That’s if this function is user facing. If it’s on the back-end I guess you could argue it’s never going to get hit as long as there are no errors in the library, so maybe that would be appropriate for assert…
But yes, as has been pointed out, your function with if...: return... elif... return... elif... return... has the problem that if I pass options='foo' the function will execute with no problem and the caller is forced to handle the returned None. This is essentially a silent failure of do_action(). Using the dispatch table the error would be caught as an exception, but the dispatch table isn’t good if the function in each case are more complicated than shown here.
I agree with @Rosuav that the code helps with extending the code in the future, or at least nicely allowing for that possibility. That’s why I wondered if NotImplementedError was more appropriate. But in most cases I have the lists I’m exhausting aren’t expected to grow to more types of implementations, so I like the ValueError.
I’ve written tests for this along the lines of
assertRaises(do_action, value=0, option='foo')
Thanks all for the useful comments. Right now I’m coding against python 3.9 and above, so can’t use match statements sadly. They definitely seem like the elegant solution here. Also the fact that case _: is idiomatic reveals that it is generally prudent to handle unmatched cases.
def do_action(value: float, option: Options):
if option is Options.IDENTITY:
new_value = value
elif option is Options.DOUBLE:
new_value = value * 2
elif options is Options.SQUARE:
new_value = value**2
else:
raise ValueError(f'Unhandled option {option}')
return new_value
From a technical standpoint, ValueError will never be raised. And if it does get raised, it’s the incorrect exception to occur. TypeError would be the suitable exception to raise.
You can prevent raising exceptions by utilizing assert isinstance(option, Options). This essentially means you can enforce typing in Python through the use of assertions.
I’m struggling to imagine any possible scenario where a subclassing float just to avoid a trivial conditional that a freshman CS undergrad could understand, would ever pass any code review. If you actually wanted to go “full-OO” here then the right way to do that would be to define separate classes for each of the actions that know how to apply themselves to their inputs. Unlike subclassing a builtin-type and dumping a bunch of unrelated new methods on, the (well-established) action pattern approach would separate concerns and could also easily be extended by clients if necessary. But I digress…
FWIW I am in the early return camp, but I like vertical space and avoid else-branches when they are superfluous (e.g. because of a return):
def do_action(value: float, option: Options) -> float:
if option == Options.IDENTITY:
return value
if option == Options.DOUBLE:
return value * 2
if option == Options.SQUARE:
return value ** 2
raise ValueError()
def get_round_digit(num: Decimal,
round_mode: RoundMode,
ndigits: Union[int, type(AutoDigits)],
pdg_sig_figs: bool = False) -> int:
if round_mode is RoundMode.SIG_FIG:
if ndigits is AutoDigits:
if pdg_sig_figs:
round_digit = get_pdg_round_digit(num)
else:
round_digit = get_bottom_digit(num)
else:
round_digit = get_top_digit(num) - (ndigits - 1)
elif round_mode is RoundMode.DEC_PLACE:
if ndigits is AutoDigits:
round_digit = get_bottom_digit(num)
else:
round_digit = -ndigits
else:
raise TypeError(f'Unhandled round mode: {round_mode}.')
return round_digit
RoundMode is an Enum whose members are SIG_FIG and DEC_PLACE and nothing else. This function uses one of two strategies to calculate round_digit which is its return value. The else is there to catch bad caller input.
I could, e.g. define round_digit = None before the if... elif... block and then have None returned in the event of bad caller input, but this would be a silent failure and the caller would have to handle the None. Similar I could use “early return” and return inside the if and elif blocks and exclude the else block, but this would have the exact same behavior of silently failing and returning None.
I think the Enum is justified in my real life example
I’m not sure what you’re saying here. I find static type analysis is helpful for writing and reading code so I’m going to go ahead and choose to use it. As for assert, my undertanding is that it is very bad practice to use assert to validate user input. This is because those checks are removed when python is run in -O or -OO mode. This means that without -O or -OO my code would raise an assertion error on bad user input but in -O or -OO mode my code would return None on bad user input. Having different behaviors based on these flags is NOT what I want…
Like I said, I can do that, but it breaks static type analysis and leads to the problem described above.
That’s correct, but assert as a hint to a type checker is fine. So if you have other protections that ensure that it really will be a valid Option, you can use assert to inform the type checker that you really have checked every possible Option. But if there’s any way for bad input to result in the assert triggering, then yes, that should be a regular exception.
My solution is bad because it assumes there are only ever two round modes and it needs a comment for clarity, but I’m posting because I had fun with it. I hope the logic is correct. I think your code as written is best.
def get_round_digit(num: Decimal,
round_mode: RoundMode,
ndigits: Union[int, type(AutoDigits)],
pdg_sig_figs: bool = False) -> int:
if ndigits is AutoDigits:
if pdg_sig_figs and round_mode is RoundMode.SIG_FIG:
round_digits = get_pdg_round_digit(num)
else:
round_digits = get_bottom_digit(num)
elif round_mode is RoundMode.SIG_FIG:
round_mode = get_top_digit(num) - (ndigits - 1)
else: # round_mode is RoundMode.DEC_PLACE and ndigits is an int
round_mode = -ndigits
return round_digits
This way, you would cover all the possible types of function arguments:
(I would suggest to use ndigits: Union[int, AutoDigits])
def get_round_digit(num: Decimal,
round_mode: RoundMode,
ndigits: Union[int, AutoDigits],
pdg_sig_figs: bool = False) -> int:
round_functions = {
RoundMode.SIG_FIG: {
AutoDigits: get_pdg_round_digit if pdg_sig_figs else get_bottom_digit,
int: lambda num: get_top_digit(num) - (ndigits - 1)
},
RoundMode.DEC_PLACE: {
AutoDigits: get_bottom_digit,
int: lambda num: -ndigits
}
}
if round_mode in round_functions:
if type(ndigits) in round_functions[round_mode]:
if callable(round_functions[round_mode][type(ndigits)]):
return round_functions[round_mode][type(ndigits)](num)
raise TypeError(f'Invalid input combination: '
f'round_mode={round_mode}, ndigits={ndigits}.')
# example usage
result = get_round_digit(
Decimal(123.456),
RoundMode.DEC_PLACE,
AutoDigits(),
True)
print(result)
If you already have a function called validate_options, this section of code becomes unnecessary:
if round_mode in round_functions:
if type(ndigits) in round_functions[round_mode]:
if callable(round_functions[round_mode][type(ndigits)]):
return round_functions[round_mode][type(ndigits)](num)
raise TypeError(f'Invalid input combination: '
f'round_mode={round_mode}, ndigits={ndigits}.')
