2
\$\begingroup\$

This script creates a an Access table that my boss uses for making a report in Access. The data comes from a comparison of data in more than a few tables in Arcmaps. Right now this script works in an awkward stage that I have to leave it at right now because I have to do other things. I have actually been working on automating some things in sql server with python and a lot of the scripts I've been looking at are written in a very different style. The primary purpose of the scripts I write is to make them easy to maintain and easy to understand for people who don't know much about python, myself included. I was wondering if my script is well styled. Here is my entire script, all the variables that equate to blank strings are correct I just can't share the strings and such with you:

##Author: Steve S. Steves

import arcpy
import os
import pyodbc
from pyodbc import Error
import time
import shutil


def compareLists(listOne, listTwo,):

    notInList = [item for item in listOne if (item not in listTwo)]
    print "Lists compared wp"
    assert notInList[0] not in listTwo
    return notInList


def createConnection(dbFile):
    print "In create_Connection function"
    for x in pyodbc.drivers():
        if x.startswith('Microsoft Access Driver'):
            driver = x
    connStr = r'DRIVER={}; DBQ={}'.format(driver,dbFile)
    conn = pyodbc.connect(connStr)
    print("Connection Created")
    return conn

def executeSql(conn, sqlStatement):
    c = conn.cursor()
    c.execute(sqlStatement)
    print "query executed"


def populateTable(conn, populateTableSql, values):
    c = conn.cursor()
    c.executemany(populateTableSql, values)


def buildingTables(dbFile, sqlStatement, populateTableSql, values):   
    print "creating connection"
    conn = createConnection(dbFile)

    try:
            print "Calling functions"        
            executeSql(conn, sqlStatement)
            populateTable(conn, populateTableSql, values)
            conn.commit()
            print "transaction committed"

    except Error as e:
        print(e)

    finally:
        #Checks to see if conn is a variable and then closes it if it is
        if conn:
            print "closing connection"
            conn.close()

#Making a FC consisting of only some things in other things
Directory = r""

# Calculate date, time, and FGDB name
date = time.strftime('%Y %m %d  %H %M %S')
GDB_Name = date + '_NewFootprints.gdb'

# Create a new FileGDB
arcpy.CreateFileGDB_management(Directory, GDB_Name)

# Set to path of created FileGDB
GDB = os.path.join(Directory, GDB_Name)

connection_name = ""
database_platform = ""
instance = ""
authentication = ""
username = ""
password = ""
savePW = ""
database = ""

# Look for folder to put y Connection into
if not os.path.isdir(Directory):
    os.path.makedirs(Directory)

# Look for y connection and create if absent
if not os.path.isfile(os.path.join(Directory, connection_name)):
    print ("Making connection file")
    arcpy.CreateDatabaseConnection_management(Directory,
                                          connection_name,
                                          database_platform,
                                          instance,
                                          authentication,
                                          username,
                                          password,
                                          savePW,
                                          database)

yFDS = r""
FeatureClass = r""
FullPathFC = os.path.join(Directory, connection_name, yFDS, FeatureClass)
sr = arcpy.Describe(FullPathFC).spatialReference

# Create new FDS for footprint fc
arcpy.CreateFeatureDataset_management(GDB,
                                      "",
                                      sr)

# Get file path of FDS in created FileGDB
newFDS = os.path.join(GDB, "")

print ("New File GDB and feature dataset have been created")

xFC = os.path.join(r"",r"")
ysFC = os.path.join(r"",r"")
cityLimitsFC = os.path.join(r"",r"",r"")


inFeaturesList = [xFC, cityLimitsFC]
outFeatureClass = os.path.join(newFDS,"")

arcpy.Intersect_analysis(inFeaturesList, outFeatureClass)
print "Intersect ran"

#list and dict comprehensions
ysFC = os.path.join(r"",r"")
ysFields = ["","","",""]
#Not using owner number
ysFieldsDict = ["","","", "", "", "", "", "", "", "", "", ""]
yList = [[str(row[0]),str(row[2]), str(row[3]), str(row[4]),
                   str(row[5]), str(row[6]), str(row[7]),str(row[8]),
                   str(row[9]), str(row[10]),str(row[11])]
                  for row in arcpy.da.SearchCursor(ysFC,
                                                   ysFieldsDict)]
print "Done with y Parcels List"

#make a function for making these lists out of the queries. 
xFC = outFeatureClass
xFields = ["","","",""]
xFieldsDict = ["","","",""]
xList = [str(row[0]) for row in arcpy.da.SearchCursor(xFC, xFields)]
z = r""
zFields = ["", "", "", "", "", "", "", "", "", "", "", ""]
zList = [[str(row[0]), str(row[2]), str(row[11]) ,str(row[3]), str(row[4]),
              str(row[5]), str(row[6]), str(row[7]), str(row[8]),
              str(row[9]), str(row[10])] for row in
             arcpy.da.SearchCursor(z, zFields)]
print "z db loaded"


#using z db to get records for xList
moreFieldsxList = [item for item in zList if item[0] in xList]

print "moreFieldsxList made"
print len(moreFieldsxList)
#getting as many aas from the thing as we can.
#faster to load entire database and use python to manipulate it rather
#than using looping search queires. 

aaFC = os.path.join(r"",r"")
#This is a for a lookup
aaFields = ["",""]

aaCodeDict = {row[1]: row[0] for row in arcpy.da.SearchCursor(
    aaFC, aaFields)}

print "aa Code dictionary built"

for item in moreFieldsxList:
    if item[2] in aaCodeDict:
        item[2] = aaCodeDict[item[2]]

print "Done with adding information to moreFieldsxList"

listOne = moreFieldsxList
listTwo = yList
listOne = tuple(listOne)
listTwo = tuple(listTwo)
print "Running compare functions"

queryListOne = compareLists(listOne, listTwo) 
queryListTwo = compareLists(listTwo, listOne)
print "List comparisons successful"

#Writing to access database. There are in fact plans to make the
#comparison functions two left joins including only results that
#don't match like it should have been in the first place.
dbFile = "U:\FileMcFileFace.mdb"
sqlStatement1 = """ CREATE TABLE In_y_Not_x(
                                    ONE TEXT,
                                    TWO TEXT,
                                    THREE TEXT,
                                    FOUR TEXT,
                                    FIVE TEXT,
                                    SIX TEXT,
                                    SEVEN TEXT,
                                    EIGHT TEXT,
                                    NINE TEXT,
                                    TEN TEXT,
                                    TENTY-ONE TEXT
                                    )
                                    """
sqlStatement2 = """ CREATE TABLE In_x_Not_y(
                                    ONE TEXT,
                                    TWO TEXT,
                                    THREE TEXT,
                                    FOUR TEXT,
                                    FIVE TEXT,
                                    SIX TEXT,
                                    SEVEN TEXT,
                                    EIGHT TEXT,
                                    NINE TEXT,
                                    TEN TEXT,
                                    ELEVEN TEXT
                                    )
                                    """
populateTableSql1 = """ INSERT INTO In_y_Not_x VALUES(
                                        ?,?,?,?,?,?,?,?,?,?,?)"""
populateTableSql2 = """ INSERT INTO In_x_Not_y VALUES(
                                        ?,?,?,?,?,?,?,?,?,?,?)"""
values1 = queryListOne
values2 = queryListTwo

buildingTables(dbFile, sqlStatement1, populateTableSql1, values1)
buildingTables(dbFile, sqlStatement2, populateTableSql2, values2)


shutil.rmtree(GDB)

print "Finished, and without any deaths too!"

Is there anything I can do to improve the readability and style in this script? Is there something that can be done to improve performance?

I'm using python 2.7 though advice for python 3 is appreciated since I'll be rewriting this script in that soon.

\$\endgroup\$
1
+50
\$\begingroup\$

There are a lot of debug prints in your code snippet. I will mark them with a D-> if I can't see any use for the end user. I would opt for removing them.

def compareLists(listOne, listTwo,):

notInList = [item for item in listOne if (item not in listTwo)]
print "Lists compared wp"
--> assert notInList[0] not in listTwo
return notInList

You can safely remove this line. If you want to do functional testing, I would move such tests into a separate tests folder to not pollute the main code. Also this test case is not even remotely sufficient ;)


def createConnection(dbFile):
  D-> print "In create_Connection function"
B --> for x in pyodbc.drivers():
B -->    if x.startswith('Microsoft Access Driver'):
B -->        driver = x
connStr = r'DRIVER={}; DBQ={}'.format(driver,dbFile)
conn = pyodbc.connect(connStr)
print("Connection Created")
return conn

B) This is not very pythonic. pyodbc.drivers() is iterable and thus you can again use list comprehension

drivers = [x for x in pyodbc.drivers() if x.startswith('Microsoft Access Driver')]
driver = drivers[-1]  # (your implementation picks the last one)

def executeSql(conn, sqlStatement):
    c = conn.cursor()
    c.execute(sqlStatement)
-->    print "query executed"

Again a debug statement. There is not much use of informing the user that a query has been executed. He cares for the side effect of this, i.e. if there have been the correct rows returned


def buildingTables(dbFile, sqlStatement, populateTableSql, values):   
D-> print "creating connection"
    conn = createConnection(dbFile)

    try:
D->     print "Calling functions"        
        executeSql(conn, sqlStatement)
        populateTable(conn, populateTableSql, values)
        conn.commit()
A -->   print "transaction committed"

B -->    except Error as e:
B -->        print(e)

    finally:
        #Checks to see if conn is a variable and then closes it if it is
        if conn:
  d->       print "closing connection"
            conn.close()

A) This is not very useful output. I would rather return how many lines have been inserted into the database or nothing at all. The fact that something happened somewhere while this code executed is implied by the user calling the script.

B) If the script fails, you should inform the user; however, it is probably better to not catch any exception and then continue. The finally block will be executed regardless of what happens in try. You can omit the entire except block. A try ... finally setup is thought of as a "cleanup block" which will make sure that the finally is executed before any exception is escalated.


# Look for folder to put y Connection into
if not os.path.isdir(Directory):
    os.path.makedirs(Directory)

Unfortunately, you didn't specify your Python version. In 3.X there is the exists_ok=False flag. Hence you could refactor this to os.path.makedirs(Directory, exists_ok=True)

You could also get access to pathlib (there are implementations for 2.7 on pip). This would allow dealing with paths in a pythonic way, because you can now do things like: Directory/connection_name rather then os.path.join(Directory, connection_name) and have python deal with the os specific bits (symlinks, correct slashes, ...)


ysFieldsDict = ["","","", "", "", "", "", "", "", "", "", ""]

But this is a list and not a dict D:. I would rename the variable.


yList = [[str(row[0]),str(row[2]), str(row[3]), str(row[4]),
               str(row[5]), str(row[6]), str(row[7]),str(row[8]),
               str(row[9]), str(row[10]),str(row[11])]
              for row in arcpy.da.SearchCursor(ysFC,
                                               ysFieldsDict)]

rather use a double list comprehension:

yList = [[str(row[x]) for x in [0,2,3,4,5,6,7,8,9,10,11]]
         for row in arcpy.da.SearchCursor(ysFC, ysFieldsDict)]

zList = [[str(row[0]), str(row[2]), str(row[11]) ,str(row[3]), str(row[4]),
          str(row[5]), str(row[6]), str(row[7]), str(row[8]),
          str(row[9]), str(row[10])] for row in
         arcpy.da.SearchCursor(z, zFields)]

same trick:

zList = [[str(row[x]) for x in [0,2,11,3,4,5,6,7,8,9,10]]
         for row in arcpy.da.SearchCursor(z, zFields)]

Joining two tables on a condition is probably more efficiently done by the database, either without additional work on your end (if the DB supports) or by maintaining a table of matches and updating upon insertion in either table using indexes. This thing can be made faster:

#using z db to get records for xList
moreFieldsxList = [item for item in zList if item[0] in xList]
\$\endgroup\$
  • \$\begingroup\$ Thanks for the feedback. I spent hours trying to fix the list comprehensions. Of course, now that I see the double list comprehension it seems so obvious now. Would you say that using pandas in this for the comparison would speed things up or slow things down? I'm in python 2.7 \$\endgroup\$ – user106363 Apr 23 '18 at 20:36
  • \$\begingroup\$ How would I write tests for this? \$\endgroup\$ – user106363 Apr 23 '18 at 20:44
  • \$\begingroup\$ Do I want to use comprehensions everywhere they can be used in order to write pythonic code? \$\endgroup\$ – user106363 Apr 23 '18 at 21:45
  • \$\begingroup\$ @Steve Yes, list comprehension is often the more pythonic approach. I don't think you would gain a lot from pandas or numpy since you are not manipulating the data in any extravagant way. You would write tests as you would for any other project that uses unit-tests or functional tests. It's a very broad question. \$\endgroup\$ – FirefoxMetzger Apr 24 '18 at 14:35
  • 1
    \$\begingroup\$ @Steve From my understanding, you are only interested in making sure the connection is closed even if there is an error; you are not actually interested in handling or modifying the error itself. So yes, in this particular case, you can rely on the trackback reporting whats wrong. If you want pretty output you can catch the single, specific error that comes with a connection problem and respond to that. You should never simply "catch all errors". \$\endgroup\$ – FirefoxMetzger Apr 25 '18 at 13:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy