10
\$\begingroup\$

Module name: database.py

Code:

import sqlite3

class Database(object):
    """sqlite3 database class that holds testers jobs"""
    DB_LOCATION = "/root/Documents/testerJobSearch/tester_db.sqlite"

    def __init__(self):
        """Initialize db class variables"""
        self.connection = sqlite3.connect(Database.DB_LOCATION)
        self.cur = self.connection.cursor()

    def close(self):
        """close sqlite3 connection"""
        self.connection.close()

    def execute(self, new_data):
        """execute a row of data to current cursor"""
        self.cur.execute(new_data)

    def executemany(self, many_new_data):
        """add many new data to database in one go"""
        self.create_table()
        self.cur.executemany('REPLACE INTO jobs VALUES(?, ?, ?, ?)', many_new_data)

    def create_table(self):
        """create a database table if it does not exist already"""
        self.cur.execute('''CREATE TABLE IF NOT EXISTS jobs(title text, \
                                                            job_id integer PRIMARY KEY, 
                                                            company text,
                                                            age integer)''')

    def commit(self):
        """commit changes to database"""
        self.connection.commit()

I have a few questions here:

  • I named my python module as database.py and my class as Database, which is perhaps not the best way, as they are too generic and somewhat less informative and they are almost identical. Is there a better way to name them?
  • I exposed the database connection and database cur as instance variables. Is that a good practice?
\$\endgroup\$
8
\$\begingroup\$

I exposed the database connection and database cur as instance variables. Is that a good practice?

Having a cursor defined as an instance variable is a common practice. The problem though here is that you don't have a way to close the cursor and the connection itself in case of an error. In situations like this, having "context manager" capabilities on your class would be quite handy:

def __enter__(self):
    return self

def __exit__(self, ext_type, exc_value, traceback):
    self.cursor.close()
    if isinstance(exc_value, Exception):
        self.connection.rollback()
    else:
        self.connection.commit()
    self.connection.close()

Then, the Database class could be used with with:

with Database() as db:
    db.create_table()
\$\endgroup\$
5
\$\begingroup\$
  1. Security

    There is a security problem in your approach because you expose the database location. The minimum thing you could do would be to set DB_LOCATION to private. I mean, something like this:

    class Database(object):
        __DB_LOCATION = "/root/Documents/testerJobSearch/tester_db.sqlite"
    

    But this is far than being enough because a good security practice is that you should save the database information in a separate configuration file (for example in YAML) and give it the right and restricted access privileges so that only your application can access to it.

  2. Connectivity approach

    A previous solution relies on __exit__() and __enter__() to facilitate coping with objects using the with statement (with Database() as db: ), but an other approach may be also convenient, depending on your context and what you want, to close the connection with the destructor method __del__()1:

    class Database:
        def __init__(self):
            self.__db_connection = sqlite3.connect(self__DB_LOCATION)
            # ...
        def __del__(self):
            self.__db_connection.close()
    

    This way the database connection is established as soon as you instantiate the object (which thing your code already does), and the connection gets lost when the instance in question becomes out of range/scope.

  3. Remove useless docstrings

    I believe most (if not all) of your docstring can be removed. Here is an example as why:

    def close(self):
        """close sqlite3 connection"""
        self.connection.close()
    

    Well: the code is self explanatory, so that docstring is mere noise and distraction for the maintainer/reader of your code. That docstring does not add any useful information about that function.

  4. Naming

I named my python module as database.py and my class as Database, which is perhaps not the best way, as they are too generic and somewhat less informative and they are almost identical. Is there a better way to name them?

Indeed, in the same way that we do not call a list of something by list, we do not call a database of testers jobs by Database. My problem is that I do not understand what you mean by "testers job" you mentioned in the docstring. Depending on what you mean by "testers job" you could rename Database() by it and database.py by something more meaningful and helpful such as testersjobs_db_operations.py (I prefer a longer helpful name than a shorter and "cute" but confusing one)


1:Safely using destructors in Python.

\$\endgroup\$
  • 2
    \$\begingroup\$ Thank you, I have completely missed it. Much appreciated. \$\endgroup\$ – Yu Zhang Dec 14 '17 at 8:53
4
\$\begingroup\$

To allow for more flexibility, I would give the user the ability to set the database location:

class Database(object):
    """sqlite3 database class that holds testers jobs"""
    DB_LOCATION = "/root/Documents/testerJobSearch/tester_db.sqlite"

    def __init__(self, db_location=None):
        """Initialize db class variables"""
        if db_location is not None:
            self.connection = sqlite3.connect(db_location)
        else:
            self.connection = sqlite3.connect(self.DB_LOCATION)
        self.cur = self.connection.cursor()

    ...
\$\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.