williballenthin / python-registry

Pure Python parser for Windows Registry hives.
Apache License 2.0
425 stars 103 forks source link

Return None instead of raising Exception #76

Open 3ev0 opened 7 years ago

3ev0 commented 7 years ago

Hi,

When requesting a (sub)key, Registry raises an exception if this key is not found. The same for values. Since actually the easiest way to check if a key exists (implicitly) to call key.subkey() and then the absence of the subkey is not really an exception in that sense. Would you consider replacing the exception-raising with returning None? or maybe add functions with that return None instead of raise exception.

Like this:

 def subkey(self, name):
        """
        Return the subkey with a given name as a RegistryKey.
        Return None if the subkey with the given name does not exist. 
        """
        if self._nkrecord.subkey_number() == 0:
            return None

        for k in self._nkrecord.subkey_list().keys():
            if k.name().lower() == name.lower():
                return RegistryKey(k)
        return None
williballenthin commented 7 years ago

Hey @3ev0

Thanks for raising this issue. You've definitely struck upon a coding style decision. I (think I) completely understand your suggestion, as I've written a bunch of APIs with the suggested style before.

However, these days, I tend to avoid this style. Let me explain why, and we can continue discussing it.

  1. I think that raising KeyError is more "pythonic". It is similar to how you interact with a dict: asking for a missing key raises an error. So, this might make the interface more familiar to use. However, this isn't a strong technical reason.

  2. More importantly, I've learned that returning more than one type of data from a function is a recipe for trouble. For example, when a function can return a string or a list, then every place that the function is called must check "is it a string or a list?". If the programmer forgets this, then inevitably, the program breaks at an inconvenient time. By extension, if a function returns a string or None, then every invocation must check "is the result None?". This is easy to forget, and leads to latent bugs. With the existing style, forgetting a try/except block is also a bug, but when the exception is generated, the programmer gets a very explicit stack trace with easy-to-find line number.

I might suggest the following wrapper for fetching subkeys using your style:

def get_subkey(key, name):
    '''
    Get a subkey with the given name, or return `None`.

    Example::

        sub = get_subkey(key, 'Installed Programs')
        if not sub:
            print('no programs installed')
            return
    ''''
    try:
        return key.subkey(name)
    except registry.RegistryKeyNotFound:
        return None

I'm certainly open to more discussion, so let me know what you think.

williballenthin commented 7 years ago

i've actually addressed this in my personal style guide here, though i wouldn't have expected you to find it (not really published anywhere): https://docs.google.com/document/d/1iRpeg-w4DtibwytUyC_dDT7IGhNGBP25-nQfuBa-Fyk/edit#heading=h.cmmkqwwtcyp2

NiKiZe commented 7 years ago

The downside of the wrapper is that it might be "the default case" that a key is missing but it still needs to be checked, I don't know how Python is in that case, but in .NET throwing a exception is very expensive so should always be avoided, And I think it is the same in Python.

However changing the existing method could break other peoples code, so my suggestion would instead be to create a new function called subkeyOrNone (and some refactoring to DRY) Alternative would be to add an optional parameter "throwOnMissing" or similar which by default is true and throws the exception otherwise it returns false.

3ev0 commented 7 years ago

Hi @williballenthin, Thanks for the quick response! I see the rational behind the coding decision. The example you mentioned is exactly how I solved it now. However, as a result for every missing key there now is an exception handling AND a check for the return value. Perhaps it depends on the usage of the library, which method (exception vs None) is most efficient. In this case, the solutions @NiKiZe proposes seem like a sensible option (to me).

williballenthin commented 7 years ago

The efficiency argument is interesting to me, so I've been doing some researching since @NiKiZe commented. Here's what I've found:

In C# (and C++ as well), raising an exception is a very expensive operation. It obliterates the cache, and the runtime has to do a ton of work to unwind the stack and find the appropriate handler. In these languages, which are really fast, using exceptions is sometimes discouraged. Python is a much more dynamic language (which actually explains why its not as fast as C#/C++), so I think that exceptions are not as "relatively slow".

Some experiments seem to show that:

To me, this is pretty reasonable. How do these results strike you all?

However changing the existing method could break other peoples code.

This is something that does worry me. Its probably not realistic to update the API in this current major version, but it could be changed for the next major version. In the mean time, we could add the routine .has_subkey(name) that might (or might not) be optimized for quickly checking existence. This would support the "unlikely subkey" check you have described.

refs: