On Thu, Jun 5, 2014 at 6:27 AM,
<ps16thypresenceisfullnessof...@gmail.com> wrote:
> I'm completely new to SQL, and recently started using SQLite in one of my 
> Python programs. I've gotten what I wanted to work, but I'm not sure if I'm 
> doing it in the best/most efficient way. I have attached some sample code and 
> would appreciate any (polite) comments about how the SQL (or Python) in it 
> could be improved. The code is written in Python 2, but I think it should 
> work in Python 3 if the 4 print statements are changed to function calls. Am 
> I correct that the function 'set_description2' should work the same way as 
> 'set_description'?

Happy to help out! But before I look into the code itself, two small
points. Firstly, you're using Google Groups, which miswraps text and
double-spaces all quoted text. This is considered to be extremely
annoying on this list/newsgroup, and while it's not technically your
fault, it will make people less inclined to read and respond to your
posts. I advise signing up for the mailing list instead:


Alternatively, you can read the comp.lang.python newsgroup in any good
newsreader, or you can use Gmane.

The second point that you may want to consider: If you think the code
is that close to Python 3 compatible, and if your Python 2 is either
version 2.6 or 2.7, just put this line at the top of your script:

from __future__ import print_function

Then you'll be using print() as a function, and your code may well
work unchanged on both versions. But do still tell us what version
you're running this under; "Python 2" is a start, but it'd be useful
to know which minor version, and which operating system. I don't think
it makes any difference here, but it's easy enough to just say "Tested
on Python 2.6 on Debian Linux" or "Tested on Python 2.7.7 on Windows
7" or somesuch.

On to the code! *plays Lord Phillip theme from 'The Beauty Stone'*

> def get_description(conn, name):
>     cur = conn.cursor()
>     cur.execute("SELECT description FROM ProgrammingLanguages WHERE Name=?",
>         (name,))
>     row = cur.fetchone()
>     if row:
>         return row[0]
>     return None

Since cur.fetchone() returns None if there's no row, you can simplify this down:
    return row and row[0]
Not everyone likes that sort of style, so take your pick.

> def set_description(conn, name, description):
>     cur = conn.cursor()
>     cur.execute("SELECT 1 FROM ProgrammingLanguages WHERE Name=?", (name,))
>     row = cur.fetchone()
>     if description:
>         with conn:
>             if not row:
>                 conn.execute("INSERT INTO ProgrammingLanguages VALUES(?,?)",
>                     (name, description))
>             else:
>                 conn.execute("UPDATE ProgrammingLanguages SET Description=? " 
> \
>                     "WHERE Name=?", (description, name))
>     elif row:
>         with conn:
>             conn.execute("DELETE FROM ProgrammingLanguages WHERE Name=?",
>                 (name,))
>     conn.commit()

There's a general principle in Python APIs that a "mode switch"
parameter isn't a good thing. Even more strongly, I would be very
surprised if attempting to set a blank description deleted the row
instead of setting the description to blank. My recommendation: Split
this into two functions, set_description and delete_language. In
delete_language, just unconditionally delete, don't bother checking
for the row's presence first.

The insert-or-update concept is a bit of a tricky one. With a simple
database, you probably don't need to worry about the race condition of
seeing there's no row, then going to insert, and finding that you
can't; but still consider an EAFP model instead - attempt the update,
and if nothing got updated, do the insert (or, conversely, attempt the
insert, and if you get back an error, update instead). That still has
a chance of a race (rows can still be inserted or deleted in the
middle), but if you do the more likely case first (do you expect to
mostly be updating or inserting?), you'll catch most of your work with
a single query, which is that bit safer (not to mention faster).

However, trying to use an SQL table as if it were a Python dict is
fundamentally hard. No matter how you work it, it won't be perfect.

> def set_description2(conn, name, description):
>     with conn:
>         if description:
>             conn.execute("INSERT OR REPLACE INTO ProgrammingLanguages " \
>                 "VALUES(?,?)", (name, description))
>         else:
>             conn.execute("DELETE FROM ProgrammingLanguages WHERE Name=?",
>                 (name,))
>     conn.commit()

In theory, yes, this will probably achieve the same as your
set_description. But since, as I said above, it's fundamentally hard
to do the "INSERT OR REPLACE" operation, there's no guarantee it'll be
any better than doing it manually; and this is something that not all
database backends support, so if you ever change to a more high-end
database (eg PostgreSQL), you'll have to recode this anyway.

This function, even more than the other, highlights the value of
splitting it into two. There's almost no code common to the two
branches - just the 'with' block (and I'm not sure what that
accomplishes here) and the commit call. May as well split them out and
make it clear when you're deleting something.

> conn.execute("CREATE TABLE IF NOT EXISTS ProgrammingLanguages(name TEXT " \
>             "PRIMARY KEY, description TEXT)")

I'd recommend using a triple-quoted string, unless you have a good
reason not to. Also, breaking it in the middle of one declaration is
usually a bad idea - makes it harder to read. Here's how I'd lay that

conn.execute("""CREATE TABLE IF NOT EXISTS ProgrammingLanguages (
    name TEXT PRIMARY KEY, description TEXT)""")

If you have more columns to add, you could break it at any comma;
sometimes, you can put logically-related columns onto one line, like

conn.execute("""create table if not exists ProgrammingLanguages (
    Name text primary key, Description text not null,
    Year_Introduced smallint not null, Year_Withdrawn smallint not
null, -- you should be able to put SQL comments here, too
    Uses_Braces boolean not null, Supports_Unicode boolean not null,
Provides_Bignums boolean not null,

(I tend to put SQL keywords in lowercase, and I generally put some
uppercase letters in field names like this. Neither change has any
significant effect - or shouldn't, with a properly SQL compliant
database - so do whatever you find more readable.)

Note, by the way, that I've peppered 'not null' everywhere. Apart from
the primary key (which is implicitly 'not null'), all fields are
nullable unless you say otherwise. Generally you don't need that, and
it's one less possibility to have to worry about. (You may want to use
'with default' so you can insert into the table without having to
specify every field.)

> set_description(conn, "Assembly",
>     "Making Easy Things Very Hard & Hard Things Impossible")

Hey, that's not fair! Assembly language makes some hard things really
easy, like segfaulting your process. Credit where it's due! :)

> set_description(conn, "Assembly", None)
> print "Assembly: %s" % get_description(conn, "Assembly")  # Should be None

This is where I would be using an explicit delete operation. The only
advantage of the merged API is that you could get "a string or None"
from somewhere and pass it straight to set_description(), and have it
figure out whether to update or delete; but in the very rare situation
where you'd do that, you can have the explicit "if desc is None" check
outside the function just as easily as inside. (And I would advise
using "if desc is None" rather than just "if desc", because a blank
description shouldn't normally be a problem - or if it is, that should
be commented somewhere.)

By and large, you have some reasonably decent code there. The points
I've made are all fairly minor, and several of them are matters of
stylistic taste more than anything else. SQL is sufficiently simple
(for simple jobs, at least) that it's hard to do anything really
horrendously wrong; you clearly know about parameterized queries,
which would be the one biggest unobviosity to learn, and other than
that... if it works, it's probably right. Once you start getting into
more complicated queries (multi-table joins, true transactional
processing, stuff like that), there are more ways to go wrong, but
even then, it's pretty easy to figure stuff out.

One final point, before I hit send. When you decide that SQLite isn't
enough for your purposes (maybe when you need multiple applications to
be able to hammer the database - SQLite isn't designed for high
throughput), skip MySQL and go straight to PostgreSQL. Even though
MySQL might look tempting at first (it has "REPLACE INTO" which does
your INSERT/UPDATE job - PostgreSQL doesn't, the SQL standard
doesn't), it's full of annoying traps, and its defaults leave a lot to
be desired. Go with PostgreSQL; the defaults may be unideal as regards
performance, but at least they're conservative as regards reliability,
and that's what you need.

All the best!


Reply via email to