2
\$\begingroup\$

I have an Oracle GIS database that has spatial tables. The records in the spatial tables have a geometry column that contains polylines.

I've written a Python 2.7.16 script that calculates the midpoint of the polylines and inserts the midpoint information into parallel tables. The script will be run as part of a nightly scheduled job on a server.

Note: I use the short form FC in the code, which stands for Feature Class. A feature class is an ESRI GIS term that can be thought of as a table with a geometry column.


What the code does:

  1. Loops through a list. Each row in the list has an input FC, a target FC, and an ID field.
  2. For each row in the list:
    • Truncate the target FC
    • Populate the target FC with midpoints from the input FC

So yeah, I'm new to Python. How can the code be improved?

import arcpy
conn = "Database Connections\\<YOUR CONNECTION.sde>\\"

#This script only works for polyline feature classes (not polygons or points).
#Caution! The target FCs will get truncated! All records in the target FCs will be deleted.

#       INPUT FC               TARGET FC                     ID FIELD (for both the input FC and the target FC)    
lstFC = [\
        ["<OWNER>.<SIDEWALK>", "<OWNER>.S_SIDEWLK_MIDPOINT", "SDW_ID"   ],\
        ["<OWNER>.<SEWER>"   , "<OWNER>.S_SEWER_MIDPOINT"  , "SEWER_ID" ],\
        ["<OWNER>.<ROAD>"    , "<OWNER>.S_ROAD_MIDPOINT"   , "ROAD_ID"  ],\
        ]

def ReloadMidpoints():
    for fc in lstFC:
        inputFC  = conn + (fc)[0]
        targetFC = conn + (fc)[1]
        idField  = (fc)[2]

        arcpy.TruncateTable_management(targetFC)
        print "<" + targetFC + "> has been truncated."

        with arcpy.da.SearchCursor(inputFC, ['SHAPE@', idField]) as s_rows:
            with arcpy.da.InsertCursor(targetFC, ['SHAPE@', idField, "MIDPOINT_X","MIDPOINT_Y"]) as i_rows:
                for row in s_rows:
                    Midpoint = row[0].positionAlongLine(0.50,True).firstPoint
                    rowVals = [(Midpoint.X,Midpoint.Y),s_rows[1],Midpoint.X,Midpoint.Y]
                    i_rows.insertRow(rowVals)

        print "<" + targetFC + "> has been reloaded with midpoints that were generated from <" + (inputFC) + ">."
        print ""

ReloadMidpoints()
print "Complete."
\$\endgroup\$
2
+50
\$\begingroup\$

I don't have any experience with arcpy, so possibly some of this may be invalid.

Mostly the code is very good: It's concise; it's clear enough what it does without lot's of documentation, and it has the documentation it needs.

Exactly what makes code "good" depends on how it's going to be used. This looks like a standalone utility you use at your work, so keeing it brief and easy to maintain is probably worth more that thoroughly documenting everything and enforcing type constraints. Not everything below will be applicable to you.

Context:

  • Use python 3.x if at all possible.
  • It's good that you've already put most of your code inside a function; now consider using the main method pattern.
  • The comment about truncating the table is good. You could move it to a confirmation prompt that would be shown to the user at runtime.

Style:

  • If you have parens of any kind (({[), you don't need to use \ to break your lines.
  • Your variable names could be more descriptive.
  • Use snake-case: this_is_a_variable
  • Be more aggressive about breaking your lines. A reader should rarely, if ever, need to scroll sideways.
  • Use parens for print statements. print("message") This is required in python 3.
  • Consider using "string".format() instead of string concatenation. (It make more of a difference in more complicated situations.)
  • You could save yourself a level of indentation by doing both your DB connections in a single with statement. (but see below)
  • The warning about truncating the table could be moved to a docstring.
  • The standard indentation scheme is 4 spaces per level of indentation, and brackets & parens close on the same level of indentation as the line they were opened on:
lstFC = [
    ["<OWNER>.<SIDEWALK>", "<OWNER>.S_SIDEWLK_MIDPOINT", "SDW_ID"   ],
    ["<OWNER>.<SEWER>"   , "<OWNER>.S_SEWER_MIDPOINT"  , "SEWER_ID" ],
    ["<OWNER>.<ROAD>"    , "<OWNER>.S_ROAD_MIDPOINT"   , "ROAD_ID"  ],
]

Architecture:

  • It looks like you could make the string literals in lstFC less verbose, by pulling out the common patterns. But that might not be a good idea if those patterns are likely to fail in the future.
  • Instead of assigning inputFC, targetFC, and idField in three steps, use unpacking to do them all at once.
  • Alternately, you could save yourself the comment about what's in lstFC by having a class that exposes those as instance properties.
  • I think it would look better, and be more flexable, to move the for fc in lstFC: loop outside def ReloadMidpoints():, so you'll be calling the function inside the loop.
  • There's probably a way to do i_rows.insertRows(rows). That way you could first load all the data, then do all the transformations, then write all the data. You'd be making a single large request against the DB, which would likely be faster, and you'd have already succeeded or failed at all of the processing before you ever touched the database. You can even avoid having two connections going on at once.
  • Speaking of DB safety, consider using a transaction? I don't know how that's done in arcpy.
  • A lot of database systems support a command called REPLACE, which is like INSERT except it will delete conflicting data before inserting the new rows. If that's an option it might be better than the truncate and rebuild pattern you're using.

To summarize a lot of what I've written:

#!/usr/bin/env python3

import arcpy

DESCRIPTION = """
Transcribe the midpoints of source feature classes
to their respective target feature classes.
This script only works for polyline feature classes (not polygons or points).
CAUTION! The target FCs will get truncated!
         All records in the target FCs will be deleted.
"""

default_connection = "Database Connections\\<YOUR CONNECTION.sde>\\"

default_feature_classes = [
    ("<OWNER>.<SIDEWALK>", "<OWNER>.S_SIDEWLK_MIDPOINT", "SDW_ID"),
    ("<OWNER>.<SEWER>", "<OWNER>.S_SEWER_MIDPOINT", "SEWER_ID"),
    ("<OWNER>.<ROAD>", "<OWNER>.S_ROAD_MIDPOINT", "ROAD_ID"),
]

def main():
    if(_confirm()):
        for (input_FC, target_FC, id_field) in default_feature_classes:
            reload_midpoints(
                default_connection + input_FC,
                default_connection + target_FC,
                id_field
            )
        print(_main_success_message)

def _confirm():
    print(DESCRIPTION)
    input(_confirmation_prompt)
    return True
    
def midpoint(row):
    return row[0].positionAlongLine(0.50,True).firstPoint
    
def reload_midpoints(input_connection, target_connection, id_field):
    """Caution! 
    The target FCs will get truncated! All records in the target FCs will be deleted."""

    new_rows = []
    with arcpy.da.SearchCursor(input_connection, ['SHAPE@', id_field]) as s_rows:
        new_rows = [
            [(m.X, m.Y), s_rows[1], m.X, m.Y]
            for m in map(midpoint, s_rows)
        ]
    
    if(new_rows):
        arcpy.TruncateTable_management(target_connection)
        print(_truncation_success_message.format(target_connection))
        with arcpy.da.InsertCursor(
            target_connection,
            ['SHAPE@', id_field, "MIDPOINT_X","MIDPOINT_Y"]
        ) as i_rows:
            i_rows.insertRows(new_rows)
        print(_midpoints_reloaded_message.format(target_connection, input_connection))
    else:
        print(_no_rows_message)
        
    print("")
    
_truncation_success_message = "<{0}> has been truncated."
_midpoints_reloaded_message = "<{0}> has been reloaded with midpoints that were generated from <{1}>."
_no_rows_message = "There are no new rows; doing nothing."
_confirmation_prompt = "Press Enter to continue..."
_main_success_message = "Complete."
    
if __name__ == "__main__":
    main()
```
| improve this answer | |
\$\endgroup\$

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.