Code review request

[MRAB]

 > """
 > Created on Tue Dec 21 13:39:41 2010
 > @author: jason
 >
 > Usage: cmd_drug_testing.py [options]...
 > Will select a random employee from the local database (located in the 
current directory)
 > and display the name by default.
 >
 > This program (Drug Testing) was written to help select employees for 
drug testing.
 > Program usage:
 >
 >     -h, --help               Displays this help info
 >     -l, --list-emplys        Lists all employees in the database
 >     -s, --select             Select employee for testing
 >     -r, --remove-emply       Delete employee from database, usage: -d 
employee_name or id
 >     -e, --edit-emply         Edit employee data in database, usage: 
-e employee_name or id
 >                                field_to_change new_value
 >     -i, --insert-emply       Insert new employee in database:
 >                                 must fit this format -- "id:or 
'','lastname, firstname', 'email',0"
 >     -f                       Display database name and full path
 >
 > This program was written by, Jason S. For the use of AA.
 >
 > """
 >
 > # import our needed modules
 > import sqlite3 as sqlite
 > import sys, getopt
 >
 > #set global vars
 > global cursor
 > global conn
 >

By default, if you bind (assign) to a name inside a function then Python 
assumes that the name is local to the function, unless you tell it 
otherwise with "global". Using "global" outside a function has no effect.
 >
 > def usage():
 >     """ this just prints the usage in the doc string"""
 >     print __doc__
 >
 > def dbconnect(sql):
 >     """handel the connction to the sqlite db  """

"handel the connction" -> "handle the connection"

 >     dbConnection = sqlite.connect("drug-test.db")
 >     #set the cursor
 >     cursor = dbConnection.cursor()
 >     try:
 >         #execute the sql passed from the other funtions and return 
the results

"funtions" -> "functions"

 >         result = cursor.execute(sql)
 >         dbConnection.commit()
 >     except sqlite.Error, e:
 >         result = e.args[0]
 >
 >     return result
 >
The Pythonic way is to return a result when successful and raise an 
exception when unsuccessful.

 >
 > def listEmployees(sql):
 >     #list all records in the database
 >     listemp = dbconnect(sql)
 >     for lst in listemp:
 >         print "%s, %s, %s, %s" % (lst[0],lst[1],lst[2],lst[3])
 >
I wouldn't pass in an SQL string. I'd pass in only the parameters (if 
any) and hide the SQL details inside the function, something like this:

     def listEmployees():
         # list all records in the database
         dbConnection = sqlite.connect(DB_NAME)
         try:
             try:
                 results = dbConnection.execute("select * from employees")
                 for lst in results:
                     print "%s, %s, %s, %s" % (lst[0], lst[1], lst[2], 
lst[3])
             except sqlite.Error, e:
                 print "ERROR:", e.args[0]
         finally:
             dbConnection.close()

I'd also be explicit about the names of the fields in the database in 
case their order changed in a later version.

 > def selectOneEmployee(sql):
 >     #select a random record from the database
 >     listemp = dbconnect(sql)
 >     for empl in listemp:
 >         print empl[0]
 >

     def selectOneEmployee():
         # select a random record from the database
         dbConnection = sqlite.connect(DB_NAME)
         try:
             try:
                 results = dbConnection.execute("SELECT name FROM 
employees ORDER BY RANDOM() LIMIT 1;")
                 print "The following employee has been selected\n"
                 for empl in results:
                     print empl[0]
             except sqlite.Error, e:
                 print "ERROR:", e.args[0]
         finally:
             dbConnection.close()

 > def removeOneEmployee(sqlchk,sqldel):
 >     #delete one employee by ID number
 >     chk = dbconnect(sqlchk)
 >
 >     if chk.fetchone() != None:
 >         #check to make sure the ID is valid in the database
 >         emply = dbconnect(sqlchk)
 >         emply = emply.fetchall()
 >         print "trying to remove employee %s" % (emply)
 >         try:
 >             dbconnect(sqldel)
 >
 >         except sqlite.Error, e:
 >             result = e.args[0]
 >             print result
 >
 >     else:
 >         print "Employees ID is invalid, please check the ID number"
 >

 >             sqlchk =
 >             sqldel = "delete from employees where id = \"%s\""  % (arg)

I'd do:

     def removeOneEmployee(emplyId):
         # delete one employee by ID number
         dbConnection = sqlite.connect(DB_NAME)
         try:
             try:
                 # check to make sure the ID is valid in the database
                 results = dbConnection.execute("select * from employees 
where id = %s", (emplyId, ))
                 if results.fetchone():
                     print "trying to remove employee %s" % emplyId
                     dbConnection.execute("delete from employees where 
id = %s", (emplyId, ))
                     dbConnection.commit()
                 else:
                     print "Employees ID is invalid, please check the ID 
number"
             except sqlite.Error, e:
                 print "ERROR:", e.args[0]
         finally:
             dbConnection.close()

Note that it's a very bad idea to build an SQL command yourself because 
of the risk of SQL injection attacks. The .execute method can do it for 
you safely.

 > def insertEmployee(sql):
 >     #insert a new employee into the database
 >     print sql
 >     try:
 >         dbconnect(sql)
 >
 >     except sqlite.Error, e:
 >         result = e.args[0]
 >         print result
 >

I'd do:

     def insertEmployee(details):
         # insert a new employee into the database
         # parse the details provided into a tuple which can be passed 
to the .execute method
         emplyDet = parse_details(details)
         print emplyDet
         dbConnection = sqlite.connect(DB_NAME)
         try:
             try:
                 dbConnection.execute("insert into employees values 
(%s)", emplyDet)
                     dbConnection.commit()
                 else:
                     print "Employees ID is invalid, please check the ID 
number"
             except sqlite.Error, e:
                 print "ERROR:", e.args[0]
         finally:
             dbConnection.close()

I wouldn't pass the details from the commandline without parsing and 
tidying them first. I'd also specify the fields explicitly in the SQL 
command.

 > def main(argv):
 >     """The purpose of this program is to select an empployee from 
this database for random drug
 >     testing. This program can also perform maintainance on same 
database."""

"empployee" -> "employee"
"maintainance" -> "maintenance"

 >     if argv == []:
 >         sql = "SELECT name FROM employees ORDER BY RANDOM() LIMIT 1;"
 >         print "The following employee has been selected\n"
 >         selectOneEmployee(sql)
 >
 >     try:
 >         #get the options passed by the user
 >         opts, args = getopt.getopt(argv, 
"hlsr:e:i:d",["Help","list-emplys","select","remove-emply=","edit-emply=","insert-emply="])
 >
 >     except getopt.GetoptError:
 >         usage()
 >         sys.exit(2)
 >
 >     #check throught the options and respond accordingly

"throught" -> "through"

 >     for opt, arg in opts:
 >         if opt in ("-h", "--help"):
 >             usage()
 >             sys.exit()

It's probably better to return from main instead of exiting.

 >         elif opt == '-d':
 >             global _debug
 >             _debug = 1

_debug isn't used anywhere else; it's not even initialised to 0. 
Incidentally, if it has only 2 possible values then it's better to use 
False and True instead of 0 and 1.

 >         elif opt in ("-l", "--list-emplys"):
 >             sql = "select * from employees"
 >             listEmployees(sql)
 >         elif opt in ("-s","--select"):
 >             sql = "SELECT name FROM employees ORDER BY RANDOM() LIMIT 1;"
 >             print "The following employee has been selected\n"
 >             selectOneEmployee(sql)

I'd do:

             selectOneEmployee()

and keep the SQL details hidden.

 >         elif opt in ("-r","--remove-emply"):
 >             if arg == "":
 >
 >                 sys.exit("You must provice the ID for the employee to 
remove")

"provice" -> "provide"

 >             sqlchk = "select * from employees where id = \"%s\"" % (arg)
 >             sqldel = "delete from employees where id = \"%s\""  % (arg)
 >             removeOneEmployee(sqlchk,sqldel)

I'd do:

             removeOneEmployee(arg)

and keep the SQL details hidden.

 >         elif opt in ("-i", "--insert-emply"):
 >             sql = "insert into employees values(%s)" % (arg)
 >             insertEmployee(sql)

I'd do:

             insertEmployee(arg)

and keep the SQL details hidden.

 >
 > if __name__ == "__main__":
 >     main(sys.argv[1:])