Code review request
Stefan Sonnenberg-Carstens
stefan.sonnenberg at pythonmeister.com
Wed Dec 22 15:24:24 EST 2010
Am 22.12.2010 19:34, schrieb Jason Staudenmayer:
> Hi All,
> I'm a python newbie so please be kind. I've been reading book after book and have written a script or two but this is my first real "program". Just looking for any suggestions and pointers. I've done some work with bash scripts and php (not OOP) a while a go. I'm not a programmer but would like to possibly expand in to it (right now an IT guy). Am I on the right track so far?
>
> The program should be documented enough to explain.
>
> """
> 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
>
>
> def usage():
> """ this just prints the usage in the doc string"""
> print __doc__
>
> def dbconnect(sql):
> """handel the connction to the sqlite db """
> 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
> result = cursor.execute(sql)
> dbConnection.commit()
> except sqlite.Error, e:
> result = e.args[0]
>
> return result
>
>
> 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])
>
> def selectOneEmployee(sql):
> #select a random record from the database
> listemp = dbconnect(sql)
> for empl in listemp:
> print empl[0]
>
> 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"
>
> 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
>
> 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."""
> 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
> for opt, arg in opts:
> if opt in ("-h", "--help"):
> usage()
> sys.exit()
> elif opt == '-d':
> global _debug
> _debug = 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)
> elif opt in ("-r","--remove-emply"):
> if arg == "":
>
> sys.exit("You must provice the ID for the employee to remove")
> sqlchk = "select * from employees where id = \"%s\"" % (arg)
> sqldel = "delete from employees where id = \"%s\"" % (arg)
> removeOneEmployee(sqlchk,sqldel)
> elif opt in ("-i", "--insert-emply"):
> sql = "insert into employees values(%s)" % (arg)
> insertEmployee(sql)
>
> if __name__ == "__main__":
> main(sys.argv[1:])
>
> ########## END ###################
>
> Thanks everyone.
>
>
> Jason
>
>
>
> ..·><((((º>
Hi Jason,
the program could be more dense.
You have several redundant code in there, too.
For example, all the *Employee functions basically just call dbconnect
and let
it execute the sql there.
dbconnect in this respect is not a really straight name, as it does more
than only
connect to a database.
You should avoid "!= None", better is "is not None".
The program flow is "awkward": if argv is empty (better say "if not argv"),
you show one employee, but then continue to parse opts.
I think the program would be more readable, if you would just handle
the different cases in an if-elseif-else-cascade.
The global statement is not needed,
But you can also try pylint, which will point some things out:
Report
78 statements analysed.
Statistics by type
type number old number difference %documented %badname
module 1 1 = 100.00 0.00
class 0 0 = 0 0
method 0 0 = 0 0
function 7 7 = 42.86 57.14
Duplication
now previous difference
nb duplicated lines 0 0 =
percent duplicated lines 0.000 0.000 =
Raw metrics
type number % previous difference
code 75 60.98 75 =
docstring 31 25.20 31 =
comment 3 2.44 3 =
empty 14 11.38 14 =
External dependencies
getopt (cmd_drug_testing)
sqlite3 (cmd_drug_testing)
Messages by category
type number previous difference
convention 23 23 =
refactor 0 0 =
warning 4 4 =
error 0 0 =
Messages
message id occurrences
C0103 8
C0301 7
C0324 4
C0111 4
W0604 2
W0612 1
W0601 1
Global evaluation
Your code has been rated at 6.54/10 (previous run: 6.54/10)
Messages
type module object line message
C cmd_drug_testing 6 Line too long (88/80)
C cmd_drug_testing 9 Line too long (82/80)
C cmd_drug_testing 15 Line too long (89/80)
C cmd_drug_testing 16 Line too long (91/80)
C cmd_drug_testing 19 Line too long (99/80)
C cmd_drug_testing 96 Line too long (95/80)
C cmd_drug_testing 105 Line too long (132/80)
W cmd_drug_testing 31 Using the global statement at the module level
W cmd_drug_testing 32 Using the global statement at the module level
C cmd_drug_testing dbconnect 41 Invalid name "dbConnection" (should
match [a-z_][a-z0-9_]{2,30}$)
C cmd_drug_testing dbconnect 48 Invalid name "e" (should match
[a-z_][a-z0-9_]{2,30}$)
C cmd_drug_testing listEmployees 54 Invalid name "listEmployees"
(should match [a-z_][a-z0-9_]{2,30}$)
C cmd_drug_testing listEmployees 54 Missing docstring
C cmd_drug_testing listEmployees 58 Comma not followed by a space
print "%s, %s, %s, %s" % (lst[0],lst[1],lst[2],lst[3]) ^^
C cmd_drug_testing selectOneEmployee 60 Invalid name
"selectOneEmployee" (should match [a-z_][a-z0-9_]{2,30}$)
C cmd_drug_testing selectOneEmployee 60 Missing docstring
C cmd_drug_testing removeOneEmployee 66 Invalid name
"removeOneEmployee" (should match [a-z_][a-z0-9_]{2,30}$)
C cmd_drug_testing removeOneEmployee 66 Missing docstring
C cmd_drug_testing removeOneEmployee 66 Comma not followed by a
space def removeOneEmployee(sqlchk,sqldel): ^^
C cmd_drug_testing removeOneEmployee 78 Invalid name "e" (should
match [a-z_][a-z0-9_]{2,30}$)
C cmd_drug_testing insertEmployee 85 Invalid name "insertEmployee"
(should match [a-z_][a-z0-9_]{2,30}$)
C cmd_drug_testing insertEmployee 85 Missing docstring
C cmd_drug_testing insertEmployee 91 Invalid name "e" (should match
[a-z_][a-z0-9_]{2,30}$)
C cmd_drug_testing main 105 Comma not followed by a space opts, args
= getopt.getopt(argv,
"hlsr:e:i:d",["Help","list-emplys","select","remove-emply=","edit-emply=","insert-emply="])
^^
W cmd_drug_testing main 117 Global variable '_debug' undefined at
the module level
C cmd_drug_testing main 132 Comma not followed by a space
removeOneEmployee(sqlchk,sqldel) ^^
W cmd_drug_testing main 105 Unused variable 'args'
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.python.org/pipermail/python-list/attachments/20101222/e8e42ec4/attachment.html>
More information about the Python-list
mailing list