5
\$\begingroup\$

I have a script that takes addresses from a CSV, and tries to find the GPS coordinates.

I am still learning Python, so comments as to general formatting (i.e. variable names, function names, etc.) are appreciated, as well as ways to tighten up the code. (I'm using the csv module and geopy).

The CSV is a single column of addresses (following are dummy addresses) saved as a .csv file:

123 Main Street, Chicago, IL 12345
3214 Overland Drive, Los Angeles, CA 49382
29 NE 9th Street, New York, NY 29384

And my .py file:

import csv
from geopy.geocoders import Nominatim
from geopy.geocoders import GoogleV3

ADDRESS_FILE = "Addresses"

def read_csv(file):
    with open(file,'r') as f:
        reader = csv.reader(f)
        address_list = list(reader)
    return address_list


def get_lat_lon(address):
    geolocator = Nominatim()
    try:
        location = geolocator.geocode(address)
        return location.latitude, location.longitude
    except Exception:
        return 0, 0


def google_lat_lon(address):
    geolocator = GoogleV3(api_key="xxxxx")
    try:
        location = geolocator.geocode(address)
        return location.latitude, location.longitude
    except Exception:
        return 0, 0

def find_coordinates(address):
    google_found = ""
    lat, lon = get_lat_lon(address)
    lat, lon = google_lat_lon(address)
    if lat == 0 and lon == 0:
        lat, lon = google_lat_lon(address)
        google_found = "Note: used Google"
    return lat, lon, google_found

def write_file(info):
    with open(ADDRESS_FILE+" with GPS.csv","w", newline='') as csvfile:
        writer = csv.writer(csvfile, delimiter = ",")
        for line in info:
            writer.writerow(line) 

def main():
    addresses = read_csv(ADDRESS_FILE+".csv")
    address_GPS = []
    for address in addresses:
        addr = address[0].replace(u'\xa0', u' ')
        lat, lon, google_found = find_coordinates(addr)
        address_GPS.append((addr,lat,lon, google_found, '=HYPERLINK("https://www.google.com/maps/search/"&B1&","&C1,"Link")'))

    for addr in address_GPS:
        print(addr[:len(addr)-1])
    write_file(address_GPS)     


if __name__ == '__main__':
    main()

Preliminary thoughts:

  1. addr = address[0].replace(u'\xa0', u' ') - I have this because for some reason when reading the CSV, \xa0 appears in the string, throwing off the lookup. I think it's due to Encoding? I created the CSV in Excel, File-> Save as .CSV (regular .csv, not the option for Mac, or whatever else).

  2. except Exception: - I think I should be more specific here? That's there in case there's no location found with the address.

  3. The if lat == 0 and lon == 0 is there in case the regular geopy lookup doesn't work, I use their Google Maps option. But say that doesn't work, and I want to use Bing or edit the address, there's probably a better way than keep on nesting that if lat == 0 and lon == 0 part.

  4. Is it okay to have my main() like I do? Or is it better to keep the lines in main() to primarily be function calls?

  5. Is it wise/good practice to have a return a, b, c, ..., z? Or is it best to return just one or two things per return?

\$\endgroup\$
4
\$\begingroup\$

Regarding your questions:

  1. No idea :-)
  2. Yes, you should be more specific. The easiest way to find out what exception it could raise is to run it without the guard and give it an address you know does not exist (I'm pretty sure the address "abdjdiehdbdocoegrvdkdoc" does not exist, for example). Note that you can catch multiple exceptions at once with except (Exception1, Exception2): (the parentheses are required) or with multiple except blocks (if you need to do different things, depending on the exception type).
  3. Yes, your current if lat == 0 and lon == 0 approach does not scale to additional lookup services. It is also currently slightly broken, since you always use Google (you use the function before the check). You could iterate through a list of lookup functions until you find one that returns valid coordinates:
LOOKUP_FUNCS = get_lat_lon, google_lat_lon

def find_coordinates(address, lookup_funcs=LOOKUP_FUNCS):
    for function in lookup_funcs:
        lat, lon = function(address)
        if (lat, lon) != (None, None):
            return lat, lon
    return None, None

Note that I assume here that the functions return None, None, instead of 0, 0, which is arguably a better value for something non-existing. Theoretically, some address could actually be at 0, 0. Note also that csv.writer translates None to "", when writing to a file (see the documentation).

  1. A main that does as much as yours is fine, IMO, if this is the only thing this script does. But you might still want to rename it to add_lat_lon(in_file, out_file) or something like this, to be able to import the actual functionality in some other script.In your new main block (or directly under the if __name__ == '__main__': guard), you could then call that function.

  2. It is perfectly fine to return tuples.

Some additional notes:

  • You should add docstrings to your functions to document what they do.
  • csv.writer has a writerows method that allows you to directly pass a list of rows. So you can just do writer.writerows(info).

  • Instead of print(addr[:len(addr)-1]) you can just do print(addr[:-1])

\$\endgroup\$
  • \$\begingroup\$ Thanks very much for your thoughts! These all make sense, and I especially like your answer for #3. However, when I try to implement that, I put LOOKUP_FUNCS = get_lat_lon, google_lat_lon at the top (under ADDRESS_FILE = ...) but get the error: NameError: name 'google_lat_lon' is not defined do I need to put that somewhere else? \$\endgroup\$ – BruceWayne Jan 19 '18 at 16:46
  • 1
    \$\begingroup\$ @BruceWayne Yes, after all your function definitions (but before the definition of find_coordinates). Python executes your file from top to bottom, so this is one of the few places where this actually matters. It needs to be defined when Python executes the function definition, not when the function is run, because it appears in it's signature. This is also why something like def f(x=[]) can produce some unexpected results, all executions of the function get the same instance of the list, because the header line is executed when the function is defined. \$\endgroup\$ – Graipher Jan 19 '18 at 17:25

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.