New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A proposed SQLite data access layer (DAL) #2

Open
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
1 participant
@mahmoud
Owner

mahmoud commented Nov 22, 2015

NB: This is a staged pull request, full of bad code (hence the branch name), so that we can see how some Python antipatterns manifest. I'll be playing the part of both the submitter and the reviewer, but the submitter (starting with the message following the italics) will write in quotes.

"I really like the LineDAL's JSONLines approach, and saw there was a stubbed out version of a SQLite implementation, so I am submitting this proposed implementation. I'm new to Python, so please let me know what you think."

@mahmoud

This comment has been minimized.

Owner

mahmoud commented Nov 22, 2015

Wow! Thank you! Unfortunately, as it stands, I see many issues with this code. The interface is correct, but there are many antipatterns in play that would hurt immediate usability, as well as long-term maintainability. Please see my comments inline.

Overall a good effort, though, and if you make the changes in future PRs, your code is sure to be accepted! Thanks again!

"""
import os

This comment has been minimized.

@mahmoud

mahmoud Nov 22, 2015

Owner

This appears to be an unused import, and while built-in, can set the wrong expectation for the following code.

import os
import sqlite3
import boltons

This comment has been minimized.

@mahmoud

mahmoud Nov 22, 2015

Owner

Another unused import, but this one is more concerning. Because it's a third-party package, this can really harm the reader's impression of the following code's reusability.

You should add PyLint to your editor. PyLint will tell you about unused imports.

class SQLiteDAL(object):
_extension = '.db'
def __init__(self, file_path):
self.file_path = file_path
self.__file_path = file_path

This comment has been minimized.

@mahmoud

mahmoud Nov 22, 2015

Owner

Do not use name mangling unless absolutely necessary (I have only used it once in 7 years). Name mangling is not private variables in Python, and is really just a workaround for a very specific inheritance corner case.

If you want to communicate to other developers that a variable is for internal use, a single underscore is preferred, e.g., self._file_path. (I wouldn't even say that is a high priority in this case. Mustn't spend all day navelgazing though.)

except:
pass
global total_count

This comment has been minimized.

@mahmoud

mahmoud Nov 22, 2015

Owner

Parallel to the comment above, the global keyword is a big yellow flag. Uses are rare enough in production code that a code comment is warranted. In this case, total_count should be an attribute on self.

query += '"' + in_dict['python']['compiler'] + '", '
query += '"' + str(in_dict['python']['have_readline'])[0] + '", '
query += '"' + str(in_dict['python']['have_ucs4'])[0] + '", '
query += '"' + str(in_dict['python']['is_64bit'])[0] + '", '

This comment has been minimized.

@mahmoud

mahmoud Nov 22, 2015

Owner

This is a very unidiomatic way to convert a bool into a string. Just use an inline 'T' if var else 'F' or a semantically meaningful converter function.

class SQLiteDAL(object):
_extension = '.db'
__file_path = None

This comment has been minimized.

@mahmoud

mahmoud Nov 22, 2015

Owner

Unlike Java and C++, class instance members do not need class-level declarations. extension is a true class-level attribute, but this attribute is destined to be overridden in __init__. This line should be removed.

total_count = 0
sqlite3.enable_callback_tracebacks(True)

This comment has been minimized.

@mahmoud

mahmoud Nov 22, 2015

Owner

This type of code at the module-level results in the infamous "import-time side effect". While the intentions may be good, this unexpected behavior can have serious implications.

It's best to leave functions like this to the modules closer to the main code entry point. In this case, because there may be other future sqlite usages in the project, I would centralize this sort of behavior in app.py.

conn.isolation_level = None # autocommit
conn.execute(query)
conn.close()
except:

This comment has been minimized.

@mahmoud

mahmoud Nov 22, 2015

Owner

This catch-all except clause is a major antipattern. Ignoring all exceptions is unpythonic. It's ok to pause a second and be specific about which exceptions you want to handle.

This block of code is not best-effort. This method should allow any exceptions raised here to propagate. The calling function can catch exceptions if it wants.

Furthermore, even though it's only four lines, so many different things happen in this try block, that I would also call this try block too large.

@@ -9,7 +9,7 @@
from clastic.errors import BadRequest
from clastic.middleware import GetParamMiddleware
from dal import LineDAL
from dal import LineDAL, SQLiteDAL

This comment has been minimized.

@mahmoud

mahmoud Nov 22, 2015

Owner

This is a good change.

NB: This is an affirmative comment. All other comments inline are critiques, unless otherwise specified. Only the sqlite_dal.py file is of material interest. Because there are so many comments, it may be useful to uncheck the "Show notes" option above to do an initial read through. Alternatively, use the "View" button in the top right of the sqlite_dal.py file below.

query += '"' + in_dict['uname'][5] + '", '
query += '"' + in_dict['username'] + '", '
query += '"' + in_dict['uuid']
query += '")'

This comment has been minimized.

@mahmoud

mahmoud Nov 22, 2015

Owner

This block of string construction of SQL has many problems:

  • It is incorrect. If one of the values includes a quote, the escaping will break, causing a SQL error at best.
  • It is insecure. String construction of SQL queries also opens you up to SQL injection.
  • It is brittle. One missing key and the whole function goes up in KeyError smoke.
  • It is hard to expand. If the schema or message is updated, keeping the orders consistent throughout will be a major issue.

At the very least use the parameters keyword argument of the execute function. Never allow inputs to modify the SQL query strings.

try:
conn = sqlite3.connect(self.__file_path)
rows = conn.execute(query).fetchall()

This comment has been minimized.

@mahmoud

mahmoud Nov 22, 2015

Owner

Connecting and executing are common among all the methods of this class. Factor out logic like this to a central method.

rows = conn.execute(query).fetchall()
return rows
except:
return -1

This comment has been minimized.

@mahmoud

mahmoud Nov 22, 2015

Owner

Apart from problems described in other comments, this simple function commits a crime against usability.

In the success case, this function returns a list. In the failure case, it returns an integer.

  • Use exceptions for errors.
  • Functions should have one return type (or one type + None). An exception to this rule is when the function allows the caller to pass in its own default or processing function.

In the except case, this function should allow the exception to propagate or return an empty list.

global total_count
total_count += 1
return

This comment has been minimized.

@mahmoud

mahmoud Nov 22, 2015

Owner

At over forty lines, this function is too long, especially for the level of other problems it has. Factor out a less brittle approach. Expect more power out of a line of Python.

@mahmoud mahmoud changed the title from A proposed SQLite DAL to A proposed SQLite data access layer (DAL) Apr 14, 2016

Repository owner locked and limited conversation to collaborators Apr 14, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.