2
\$\begingroup\$

Given the following working code:

def get(self, key, default=None, type=None):
    """Return the default value if the requested data doesn't exist.
    If `type` is provided and is a callable it should convert the value,
    return it or raise a :exc:`ValueError` if that is not possible.  In
    this case the function will return the default as if the value was not
    found:

    >>> d = TypeConversionDict(foo='42', bar='blub')
    >>> d.get('foo', type=int)
    42
    >>> d.get('bar', -1, type=int)
    -1

    :param key: The key to be looked up.
    :param default: The default value to be returned if the key can't
                    be looked up.  If not further specified `None` is
                    returned.
    :param type: A callable that is used to cast the value in the
                 :class:`MultiDict`.  If a :exc:`ValueError` is raised
                 by this callable the default value is returned.
    """
    try:
        rv = self[key]
        if type is not None:
            rv = type(rv)
    except (KeyError, ValueError):
        rv = default
    return rv

While it is clear that if we attempt to get a the value of a non-existent key, we will get a KeyError, it is not immediately discernible what will lead to a ValueError. It's only when we pass a string that does not contain an int do we see that this is where type() fails, and a ValueError is caught and handled.

How many actions to try before calling except? Should it not be written more explicitly and with more granularity thus:

try:
    rv = self[key]
except KeyError:
    rv = default
try:
    if type is not None:
        rv = type(rv)
except ValueError:
    rv = default
return rv
\$\endgroup\$
4
\$\begingroup\$

I prefer the explicit form of the code, because we’re minimising the amount of code that can throw. It makes it very clear exactly where we expect the exception to occur, and we don’t risk catching an unexpected exception that’s hiding in our try block.


As it happens, we can actually do away with the first try/except block. This operation – look up a key, if it’s missing, use a default – is common enough that it’s covered by the standard library; specifically, dict.get(). You can use the single line:

rv = self.get(key, default)


Separately, I’d rename the type keyword. It’s better to avoid using the same names as the built-in functions – it can be a source of subtle bugs, and just causes confusion.

\$\endgroup\$
  • \$\begingroup\$ +1 to the type problem. I thought you were trying to use the builtin and it made your code look like nonsense. \$\endgroup\$ – SuperBiasedMan Jan 25 '16 at 12:17
1
\$\begingroup\$

I agree about the explicit code. I'd add another note that you should return from the exceptions directly.

try:
    rv = self[key]
except KeyError:
    return default
try:
    if type is not None:
        rv = type(rv)
except ValueError:
    return default
return rv

The only change this has is that if a default of the wrong type is supplied then the type validation wont happen. But that's the user's problem, as they've supplied a default that doesn't suit their intended type. It makes it clearer what's happening by directly returning the default from the function.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.