Cameron,
took me some time to get to this.
Waling down your comments:
>...Normally I would have your DB class represent an open (or openable, if
you wanted to defer that) database connection. So your main class would go:
> def __init__(self, ...other args...):
> self.db = DB(location="blah.sqlite")
that was my intention with (classA)
self.DBConnection = sql3.Connection
What is puzzling me is what type should i assign to the self.DB _if_ i
were to make it (self.db) a class field?
I do not plan to have various DB connections, just one connection.
The rest of your remarks have been accepted :) Thank you very much!
On Sun, Jul 2, 2017 at 8:14 PM, Cameron Simpson <c...@zip.com.au> wrote:
> On 02Jul2017 11:02, Andrew Z <form...@gmail.com> wrote:
>
>> I'd appreciate your suggestions for a better approach to the following
>> task.
>>
>> I have 2 files ( 2 classes). One (ClassA) has all logic related to the
>> main workflow of the program. Another (DB), I like to offload all
>> operations with a DB ( sql3 in this case).
>>
>> I'm trying to pass the connection to the main class, but having problems.
>> One of them, is i can't pass the conn as a parameter to the function in one
>> (ClassA.abc()), because i inherit it ( function abc() ).
>> I created a self.DBConnection field, but i'm not sure if i'm on the right
>> path...
>> Code is gutted to highlight the problem.
>>
>
> Unfortunately you have gutted the "writeTicks" method, making it harder to
> see your intent.
>
> You separation is ok, but normally one would try to entire conceal the
> unerlying db connection (the sqlite3 connection) from the man class. So you
> wouldn't "pass the connection to the main class".
>
> Normally I would have your DB class represent an open (or openable, if you
> wanted to defer that) database connection. So your main class would go:
>
> def __init__(self, ...other args...):
> self.db = DB(location="blah.sqlite")
>
> def abc(self, reqId: int):
> self.db.writeTicks(reqId)
>
> I wouldn't be passing in "self" (your ClassA instance) or
> self.DBconnection at all. You'd only pass "self" if the "DB" instance
> needed more information from ClassA; normally you'd just pass that
> information to writeTicks() along with reqId, so that the DB needs no
> special knowledge about ClassA.
>
> I've also got a bunch of fine grained remarks about your code that you can
> take or leave as you see fit:
>
> one.py:
>> from .DB import *
>>
>
> Try to avoid importing "*". It sucks all the names from "DB" into your own
> namespace. Arguably you only need the DB class itself - all the other
> functionality comes with it as methods on the class. So:
>
> from DB import DB
>
> class ClassA(OtherObject):
>> def __init__(self):
>> self.DBConnection = sql3.Connection
>>
>
> It isn't obvious why you need this. In my example above I'd just make a DB
> instance and save it as self.db; unless you're controlling different
> backends that would be all you need.
>
> def abc(self, reqId: int):
>> DB.writeTicks(self,self.DBConnection,reqId))
>>
>
> Here you're calling the writeTicks method on the DB class itself. I
> wouldn't be making that a class method; I'd make it an instance method on a
> DB instance, so:
>
> self.db.writeTicks(reqId)
>
> unless there's more to writeTicks (which you've left out).
>
> DB.py:
>>
>
> Try not to name modules that same as their classes - it leads to
> confusion. I'd call it "db.py" and make the earlier import:
>
> from db import DB
>
> import sqlite3 as sql3
>>
>
> This feels like an pointless abbreviation.
>
> [...]
>
>> class DB(object):
>> db_location = ''
>> # db_location = '../DB/pairs.db'
>>
>
> db_location appears to be a class default. These are normally treats as
> one would a "constant" in other languages. Stylisticly, this normally means
> you'd write the name in upper case, eg:
>
> DEFAULT_DB_LOCATION = '../DB/pairs.db'
>
> def __init__(self, location='../DB/pairs.db'):
>> db_location = location
>>
>
> And using that would normally look like this:
>
> def __init__(self, location=None):
> if location is None:
> location = self.DEFAULT_DB_LOCATION
>
> print(current_fn_name(),' self.db_location =
>> {}'.format(db_location))
>> try:
>> with open(db_location) as file:
>> pass
>> except IOError as e:
>> print("Unable to locate the Db @
>> {}".format(db_location))
>>
>
> I'd just os.path.exists(db_location) myself, or outright make the db
> connection immediately.
>
> Also, and this actually is important, error messages should got the the
> program's standard error output (or to some logging system). So your print
> would look like:
>
> print("Unable to locate the Db @ {}".format(db_location),
> file=sys.stderr)
>
> Also, normal Python practie here would not be to issue an error message,
> but to raise an exception. That way the caller gets to see the problem, and
> also the caller cannot accidentally start other work in the false belief
> that the DB instance has been made successfully. So better would be:
>
> raise ValueError("Unable to locate the Db @ {}".format(db_location))
>
> def reqConnection(self):
>> try:
>> con = sql3.connect(self.db_location)
>> con.text_factory = str
>> except sql3.Error as e:
>> print("Error %s:".format( e.args[0]))
>> sys.exit(1)
>>
>
> It is generally bad for a class method (or, usually, any funtion) to abort
> the program. Raise an exception; that way (a) the caller gets to see the
> actual cause of the problem and (b) the caller can decide to abort or try
> to recover and (c) if the caller does nothing the program will abort on its
> own, doing this for free.
>
> Effectively you have embedded "polciy" inside your reqConnection method,
> generally unwelcome - it removes the ability for the caller to implement
> their own policy. And that is an architectural thing (where the policy
> lies).
>
> return con
>>
>
> The easy way to raise the exception here is just to not try/except at all,
> thus:
>
> def reqConnection(self):
> return sql3.connect(self.db_location)
>
> or if you really need that text_factory:
>
> def reqConnection(self):
> con = sql3.connect(self.db_location)
> con.text_factory = str
> return con
>
> def write(self, con : sql3.Connection, tickId: int):
>> con.execute( blah)
>>
>
> However I'd make the connection a singleton attribute of the DB class. So
> I'd usually have __init__ make the connection immediately (which saves you
> having to "probe" the location:
>
> def __init__(self, ...):
> ...
> self.con = sql3.connect(self.db_location)
>
> and then write() would go:
>
> def write(self, tickId: int):
> self.con.execute(blah)
>
> and as you can see that _removes_ any need to pass the connection back to
> the caller - you don't need to expose an reqConnection method at all, or
> manage it in the caller. Instead, ClassA can just store the DB instance
> itself, and let DB look after all the specifics. That is exactly the kind
> of thing class encapsulation is meant to achieve: the caller (Class A) can
> wash its hands of the mechanisms, which are not its problem.
>
> Cheers,
> Cameron Simpson <c...@zip.com.au>
>
--
https://mail.python.org/mailman/listinfo/python-list
