Proper architecture

[Cameron Simpson]

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>