On Fri, Jun 6, 2014 at 3:42 AM, R Johnson
<ps16thypresenceisfullnessof...@gmail.com> wrote:
> Thank you all for your replies and suggestions.
> To Chris's "two small points":
> I saw that using the mailing list was recommended to several other people
> who posted here using Google Groups, so I thought it might be recommended to
> me as well sometime :). I'll try to use it from now on.
> My code was tested on Python 2.7.6 on Windows 8.1 (and I just installed
> Python 2.7.7 yesterday).

Thanks! Good to know.

>> 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.
> I agree for the case of the sample code I showed here (which was really just
> a scaled-down version of some of the functions in my program). But in my
> actual program, I am using SQLite to load and save information from a
> wxPython GUI, where it's more practical to call a single save function.
> Below is the actual function (that's part of a class in my program):
> def save_text(self):
>     if not self.editor.IsModified():
>         return
>     if not self.editor.IsEmpty():
>         stream = cStringIO.StringIO()
>         self.editor.GetBuffer().SaveStream(stream,
>             richtext.RICHTEXT_TYPE_XML)
>         self.conn.execute("REPLACE INTO notes VALUES(?,?)",
>             (self.db_key, stream.getvalue()))
>         self.editor.SetModified(False)
>     else:
>         self.conn.execute("DELETE FROM notes WHERE topic=?",
>             (self.db_key,))

I was actually going to consider saying "don't wrap stuff up in
functions like this at all, just do your SQL queries directly in
whatever needs them", but apparently that's what you're already doing.
The mode-switch is right where it should be, at the point where the
switch happens. Your code says "if the editor is empty, do something
different", which is the easiest and clearest way to describe this.

>> > 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! :)
> OK, I'll admit that I don't know Assembly :). How about the paradox "Making
> Easy Things Hard & Hard Things Easy"? Although that might make my
> description of C++ too unfair; suggestions for improvements to my language
> descriptions are welcome :).

Hehe. As I'm sure you're aware, this has absolutely nothing to do with
your SQL or Python code.

>>        While /maybe/ not required for a SELECT operation, I'd put a
>>conn.commit() somewhere in there before the return(s). The standard for
>> Python DB-API interfaces is that auto-commit is turned off -- meaning the
>> SELECT has started a database transaction.
> I don't exactly understand why conn.commit() should be called there. I
> thought it's only necessary to call it when the database has been changed,
> which a SELECT call doesn't do. Am I misunderstanding something here?

I can't speak for SQLite3, but here's how it is with full-on databases
like DB2 and PostgreSQL. (I suspect sqlite may be a little simplified
from this, but it's likely to be the same. In any case, get into good
habits now and you won't run into problems later.) Whenever you start
working with a database, you open a unit of work, aka a transaction.
This transaction continues until it is terminated, either by a commit
or rollback, or by something breaking your connection to the db (if
your program or the database shuts down abruptly, your transaction
will be rolled back). While it's alive, it holds certain resources,
mainly locks. It's fairly obvious that updating should acquire a lock
- if some other program tries to update the same row of the same
table, it should be blocked - but even reading will grab some locks.
Depending on the database and the config, your reads might prevent
anyone else from writing, or (as with PostgreSQL) they might simply
prevent someone else from dropping that table altogether, but there'll
still be locks.

The most important thing to remember is that your commit/rollback
points should match the logical unit of work that you're doing. Figure
out what your program would do if it got shut down abruptly in the
middle of the database work - if there's any way that something could
be half-done and illogical, you should avoid committing in there,
until you've brought the database back to a stable state. Often that
means never committing until the very end of the program flow;
sometimes it means committing every little piece you do; it could be
anywhere in between.

>> >        with conn:
>>         This isn't really doing anything useful. You aren't opening a new
>> connection object, so there isn't really anything to close on block exit.
> See
> https://docs.python.org/2/library/sqlite3.html#using-the-connection-as-a-context-manager.
> I removed it from my code, though, because it doesn't really seem necessary.

It's one way of doing the commit or rollback, so it is of value.

> I've attached some new sample code in which I've attempted to correct
> various things that you mentioned. The links Peter pointed to were also
> helpful to show me some improvements I could make to my code. I'd be happy
> to hear any suggestions that anyone may have to improve the code further.

It's generally easier to include the code in the body of the message,
rather than as an attachment. That way, we can quote it and respond to
pieces, just as with your English code.

I'd avoid using the backslash continuation operator anywhere, as a
general rule. Use multiline strings (triple-quoted) or rely on
parentheses; the backslash tends to get a bit lost in some
circumstances, and you can trip yourself up badly by adding a comment
after it. Otherwise, looks fairly alright!


Reply via email to