Extract data from a DBMS, create a CSV dump and transfer it to an FTP server

First of all, my code does work as expected at least to some extent. The intention of the program is to extract data from a DBMS in order to create a CSV dump and transfer it to an FTP server.

My two issues:

• The functions of the class seem to be bloated with self.-references. How can that be optimized? Or is that the way it ought to be? It looks overloaded to me.

• The population of the plist-variable (first call of if __name__ == '__main__':) has been done very ineptly. The main function does not exit the program if the number of arguments does not equal 2. How can this be achieved respectively or be done better?

```
import cx_Oracle
import re
import os
import csv
import sys
import getopt
import ftplib
import ntpath

#os.environ['NLS_LANG'] = "en_US.ISO8859-1"
#os.environ['NLS_LANG'] = ".UTF8"

def main(argv):
inputfile = ''
outputfile = ''
usage = 'test.py -i -o '
flist = []
try:
opts, args = getopt.getopt(argv,"hi:o:",["ifile=","ofile="])
except getopt.GetoptError:
print usage
sys.exit(2)
for opt, arg in opts:
if opt == '-h':
print 'test.py -i -o '
sys.exit()
elif opt in ("-i", "--ifile"):
inputfile = arg
elif opt in ("-o", "--ofile"):
outputfile = arg
elif len(sys.argv) != 2:
print usage
sys.exit(2)
flist.append(inputfile)
flist.append(outputfile)
return flist

class Extractor(object):

def __init__(self, iput, oput):
self.oracle_server = "my_oracle_server"
self.oracle_password = "my_oracle_password"
self.oracle_port = "1521"
self.oracle_sid = "MYSID"
self.oracle_user = "MYUSER"
self.ftp_server = 'my_ftp_psw'
self.ftp_user = 'my_ftp_user'
self.ftp_passwort = 'my_ftp_passwort'
self.sql_file = os.path.join(os.getcwd(),iput)
self.csv_file = os.path.join(os.getcwd(),oput)
self.dsnStr = cx_Oracle.ma

Class members

Don't use class members if local variables are enough. For example here:

def execute_sql(self, file):
    self.f = open(file)
    self.sql_statement = self.f.read()
    self.cursor.execute(self.sql_statement)

The variables f and sql_statement are used only in this function, so they should be local variables (drop the .self).

As a general rule, it's good to limit the scope of variables, to minimize the chance of accidental modification somewhere else. This is good encapsulation and information hiding.

Review the other functions too, similarly.

Use context managers whenever possible

The function in the previous example should be rewritten using a context manager:

def execute_sql(self, file):
    with open(file) as fh:
        self.cursor.execute(fh.read())

Encapsulation

Looking at this sequence of statements:

ext = Extractor(plist[0], plist[1])
ext.execute_sql(ext.sql_file)
ext.extract_from_db()
ext.cursor.close()
ext.connection.close()
ext.transfer_file()

This code knows too much about the implementation of Extractor: it knows that the class has a cursor member and a connection member. It also knows that these members need to be closed after using, and in a specific order. That's really too intimate knowledge about the implementation. It's also hard to remember these steps, every time you might use this class.

The purpose of encapsulation and information hiding is to free users from knowing too much details. The problem with "knowing" is that it means that if you change the implementation later, all callers must change too. I suggest to reorganize the code, so that the usage looks more like this:

ext = Extractor(plist[0], plist[1])
ext.execute_sql_from_file()
ext.extract_from_db_to_file()
ext.transfer_file()

Inside the implementation of these functions, feel free to do whatever is necessary, such as opening database connection, running query, closing cursor and connection. Users don't need to know these details. This way, you will be able to change the implantation details later, and users of the class don't have to notice anything.

StackExchange Code Review Q#134152, answer score: 3