On 04/29/2015 09:50 AM, Peter Otten wrote:
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).
I'll think about it. I can't see for now how I would combine the two
(each are triggered by different sets of conditions), but there's
probably a better way.
(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
I read that but I can't recall for the life of me why I didn't use it
that way. Maybe I didn't understand it well or something broke, etc.
I'll look into it.
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>
).
Yes, that's the way it was but I didn't like conn.cursor and I didn't
know how to use context managers (still don't). So I wanted to have
something working first, and then improving it now.
(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.
Right. This was also pointed by Alan so it must be really shabby. I'll
look into it.
(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.
Nice, really nice :)
(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.
I have simple scripts that test for specific things, but it's not really
professional neither does it follow any structure, etc.
I'll improve the code following the recommendations on this thread..
Thanks, Peter.
--
~Jugurtha Hadjar,
_______________________________________________
Tutor maillist - Tutor@python.org
To unsubscribe or change subscription options:
https://mail.python.org/mailman/listinfo/tutor
