Proper architecture

Sun Jul 2 21:26:30 EDT 2017

Cameron,
This is much more than I hoped for.
>From quickly looking over - most your notes are perfectly on target.
Allow sometime to digest and reply. Thank you very much!
On 2 Jul 2017 8:14 p.m., "Cameron Simpson" <cs at zip.com.au> wrote:
> On 02Jul2017 11:02, Andrew Z <formisc at 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 <cs at zip.com.au>
>