[Tutor] Good Taste Question: Using SQLite3 in Python

[Peter Otten]

Jugurtha Hadjar wrote:

> I have a class with methods that access a database (SQLite3). I have 
> included an excerpt showin reading and writing and would like to know if 
> I'm doing it right. (i.e: Is it bad code and what to improve).
> 
> Here are some improvements and my rationale (check my thinking):
> 
> - Initially, each method had its SQL statement(s) inside, but I grouped 
> all statements in a dictionary, with operations as keys, as a class 
> 'constant' as per previous advice on this mailing list.
> 
> - Each method used sqlite3 module on its own, but it was repetitive so I 
> put that part in its own method `init_db` that returns a tuple 
> consisting in a connection and a cursor.
> 
> - Sometimes there was an exception raised, so I used `try` in `init_db`.
> 
> - Methods closed the connection themselves, so I used `with` in 
> `init_db` instead of `try`, as it would close the connection 
> automatically and rollback (I hope I'm not making this up).
> 
> Here's the excerpt (`DB_FILES` and `QUERIES` are not included here for 
> more clarity).
> 
> Thank you.
> 
> 
> 
>         def __init__(self, phone):
> 
>                 # Get preliminary information on user and make them
>                 # available.
> 
>                 self.phone = phone
>                 self.known = self.find()
>                 
>                 if self.known:
>                         self.balance = self.get_balance()
>                 else:
>>                         self.balance = None
> 
>         def init_db(self):
>                 with sqlite3.connect(self.DB_FILE) as conn:
>                         return conn, conn.cursor()
> 
>         def find(self):
>                 '''Find the phone in the users database.'''
>                 
>                 (__, cursor) = self.init_db()
>                 try:
>                         cursor.execute(
>                                 self.QUERIES['FIND_PHONE'],
>                                 (self.phone,)
>                         )
>                         found = cursor.fetchone()
>                         return True if found else False
>                 except Exception as e:
>                         return self.ERROR.format(e.args[0])
> 
>         def create(self, seed_balance):
>                 ''' Create a database entry for the sender.'''
> 
>                 conn, cursor = self.init_db()
>                 try:
>                         cursor.execute(
>                                 self.QUERIES['CREATE'],
>                                 (self.phone, seed_balan>ce)
>                         )
>                         conn.commit()
>                 except Exception as e:
>                         return self.ERROR.format(e.args[0])

My random observations:

(1) find() and create() look very similar. Try hard to factor out common 
code. You might even combine it into a single method ensure_balance(phone).

(2) According to

https://docs.python.org/dev/library/sqlite3.html#using-the-connection-as-a-context-manager

- the context manager automatically commits
- you can run sql directly on the connection

It might by a good idea to refactor init_db() to just return the connection 
and then use it as

    with self.init_db() as conn:
        return conn.execute(
            self.QUERIES['FIND_PHONE'], 
            (self.phone,)).fetchone() is not None

If you need a cursor you can easily get one from the connection. If you want 
more complex handling later you can always turn init_db() into a 
contextmanager (see
<https://docs.python.org/dev/library/contextlib.html#contextlib.contextmanager> 
). 

(3) Catching Exception is overly broad. You will also catch a typo like

cursor.execute(
    self.QUERIES['CERATE'],
    (self.phone, seed_balance)
)

where the proper fix is to modify the script. Only catch exceptions that you 
can actually handle. Example: a table doesn't exist, and you want to create 
it lazily on first access. Let all other exceptions just bubble up. It may 
seem lazy, but a complete traceback is invaluable for identifying and fixing 
a bug.

(4) self.ERROR.format(e.args[0])

is probably a string with len() > 0, and thus True in a boolean context. 
>From that follows an exception in the find() method in

>                 self.known = self.find()

causes the following if-suite to be run

>                 if self.known:
>                         self.balance = self.get_balance()

and if get_balance() has a same idea of proper exception handling

self.balance will end up containing an error message.

(5) From point (4) I conclude that you don't have unit tests that cover your 
code. You should really write some of these before pondering about stylistic 
issues. Unit tests 

- help avoid errors
- make changes in the code less of a gamble because if the tests succeed 
after the change you can be confident the change didn't introduce an error
- what you might not expect: how drastically tests affect the style of your 
code. You'll see yourself thinking "How can I test that?" with every line of 
code that you write, and that alone will improve the quality of your code.

A simple example is your self.find(). How would you test that with different 
phone numbers? That's easier when you can pass the phone number as an 
argument, so you might change the method's signature.