4
\$\begingroup\$

This is a part of a project I'm working on. The codebase is Python 2.7 and I don't have an option to switch to Python 3.

Code:

import shelve
def get_document_info():
    token_shelf = False

    try:
        token_shelf = shelve.open(os.path.join(current_dir, 'document_info.shelve'))
        token_mapping = token_shelf['token_mapping']

        if token_mapping:
            return token_mapping
        else:
            return False

    except Exception, e:
        pass

    finally:
        if token_shelf:
            token_shelf.close()

Shelve is a persistent, dictionary-like object. More info can be found at https://docs.python.org/2/library/shelve.html

I need to ensure that token_shelf is closed at the end. In the beginning of the function, what do I define token_shelf as? If I don't define something there, PyCharm will throw an error saying that the token_shelf in finally block does not exist.

What's the ideal way to write this piece of code? How can it be made more elegant and Pythonic?

PS: This code is working as expected right now.

\$\endgroup\$
  • \$\begingroup\$ Welcome to codereview! What is shelve? A file? Please post all the code or at least add some context about what this code is supposed to do. And it's python 2.x or 3.x? Is this your code? So many questions... \$\endgroup\$ – Grajdeanu Alex. Jan 13 '18 at 10:15
  • \$\begingroup\$ @MrGrj: Updated the post. Is there anything else I'm missing? Please let me know. \$\endgroup\$ – UnderpoweredNinja Jan 13 '18 at 10:23
  • \$\begingroup\$ Please add the import too. More, does this code work as expected? \$\endgroup\$ – Grajdeanu Alex. Jan 13 '18 at 10:35
  • 1
    \$\begingroup\$ This is not stackoverflow. How does a shelve file look like? What is current_dir? Where is the os imported? I don't see it in the code -> I can't run the code -> the question is off-topic. If you want help from us, you first have to let us help you by understanding all your code \$\endgroup\$ – Grajdeanu Alex. Jan 13 '18 at 10:59
  • \$\begingroup\$ Which exceptions are you guarding against? \$\endgroup\$ – Graipher Jan 13 '18 at 11:51
4
\$\begingroup\$

Ideally you would want to use Python's with ... as syntax, which creates a context that calls the exit method of whatever object you create. For this, the shelve.Shelf classes would need to implement the dunder methods __enter__ and __exit__ (which they don't).

At this point you basically have three options:

  1. Inherit from them and implement it yourself:

class Shelf(shelve.Shelf):

    def __new__(cls, file_name):
        return shelve.open(file_name)

    def __enter__(self):
        return self

    def __exit__(self, *args):
        self.close()
  1. Or just write a simple wrapper class like this:

class Shelf(object):

    def __init__(self, file_name):
        self.file_name = file_name

    def __enter__(self):
        self.obj = shelve.open(self.file_name)
        return self.obj

    def __exit__(self, *args):
        self.obj.close()

Both can be used like this:

import os
import shelve

def get_document_info(current_dir):
    shelf_path = os.path.join(current_dir, 'document_info.shelve')
    with Shelf(shelf_path) as token_shelf:
        return token_shelf.get('token_mapping')

When you use try..except you should always guard against as specific exceptions as possible (so that unexpected exceptions can still rise to the top). In this case this would probably be a KeyError. Here I used the get method instead, which will return None if the key is not found.

I also removed the return False, because you should check for falsiness in the calling code. In other words, if you are doing if token_mapping is False or if token_mapping == False in the calling code, you are doing it wrong. Just have the if not token_mapping/if token_mapping there.

  1. Use Python's contextlib.closing, which already implements something like option 2:

from contextlib import closing
import os
import shelve

def get_document_info(current_dir):
    shelf_path = os.path.join(current_dir, 'document_info.shelve')
    with closing(shelve.open(shelf_path)) as token_shelf:
        return token_shelf.get('token_mapping')
\$\endgroup\$
  • \$\begingroup\$ Nice. That's indeed the correct way to do what OP requested! +1 \$\endgroup\$ – Grajdeanu Alex. Jan 13 '18 at 12:29
  • \$\begingroup\$ Could you explain why in the Shelf class __new__ is implemented, and not __init__? \$\endgroup\$ – mkrieger1 Jan 13 '18 at 12:56
  • \$\begingroup\$ @mkrieger1 Because I inherit from shelve.Shelf, so it is already implemented. Also note that the open function is not a method of the original class, but a module function, so I needed some way to return the right object upon creation. It would also have been possible to set self.file_name in __init__, though and do the opening in the __enter__ method. \$\endgroup\$ – Graipher Jan 13 '18 at 13:20
  • \$\begingroup\$ @mkrieger1 I added the version without overwriting __new__. \$\endgroup\$ – Graipher Jan 13 '18 at 13:52
-3
\$\begingroup\$
def get_document_info():

global token_mapping

try:
    return token_mapping if (token_mapping = shelve.open(os.path.join(current_dir, 'document_info.shelve'))['token_mapping']) else None

except Exception, e:
    token_shelf.close()
    pass

Forcibly changed your return of False as that is bad practice as you can't substitute the shelve object everywhere unless you pre-wrap it in conditionals -- always. Honestly been a decade since I wrote anything in Python. So this is what I have pieced together from quick look-ups on tutorials online. Perhaps Python has more elegantly looking Ternary Operators (Perl is nice because it has default-ly named variables).

But, even then I never forgot that Python is a Dynamically-Typed language and that every variable is just a Reference to an instance and every instance is Strongly-typed.

\$\endgroup\$
  • 3
    \$\begingroup\$ Please keep your negative thoughts about people to yourself. Remember to be nice. \$\endgroup\$ – Simon Forsberg Jan 13 '18 at 14:32

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.