5
\$\begingroup\$

I'm looking for a code review of this python I wrote - this code reads the zipcode column value from a CSV file and calls an API to retrieve lat, long, state and city info.

It works fine and gives me the correct results, but I'm looking for ways to improve the code/my approach and exception handling. I am pretty sure there are better ways to write this long code.

All comments and suggestions as brutal as may be are mostly appreciated.

import requests
import pandas as pd
import time
from ratelimiter import RateLimiter
API_KEY = "some_key"
zip_code_col_name = "zipcode"
RETURN_FULL_RESULTS = False
excel_data = pd.read_csv("Downloads/test_order.csv", encoding='utf8')
if zip_code_col_name not in excel_data.columns:
    raise ValueError("Missing zipcode column")
# This will put all zipcodes from column to list including duplicates , hence avoiding it.
#zipcodes = excel_data[zip_code_col_name].tolist() 
zipcodes =[]
for i in excel_data.zipcode:
    if i not in zipcodes:
        zipcodes.append(i)

def get_geo_info(zipcode, API_KEY, return_full_response=False):

    init_url = "some_url" 
    if API_KEY is not None:
        url = init_url+API_KEY+"/info.json/"+format(zipcode)+"/degrees"

    # Ping site for the reuslts:
    r= requests.get(url)
    if r.status_code != 200:
        #this will print error code to but in reuslt set it will be empty
        print("error is" + str(r.status_code))
    data = r.json()
    output = {
            "zipcode" : data.get('zip_code'),
        "lat" : data.get('lat'),
         "lng" : data.get('lng'),
        "city" : data.get('city'),
        "state" : data.get('state')

            }

    return output

def limited(until):
    duration = int(round(until - time.time()))
    print('Rate limited, sleeping for {:d} seconds'.format(duration))

results = []

### Putting max call to 49 for 1 hour, we can paramterized it also
rate_limiter = RateLimiter(max_calls=49, period=3600, callback=limited)

for zipcode in zipcodes:
    with rate_limiter:
        geocode_result = get_geo_info(zipcode, API_KEY, return_full_response=RETURN_FULL_RESULTS)
        results.append(geocode_result)
#print(results)
ddf = pd.DataFrame(results)
### This is to put the geo_info into same file however I would prefer it to write it to antoher file load it int staging table. to keep it for future refrence purposes.
""""df = pd.read_csv("Downloads/test_order.csv")
df['state'] = ddf.state
df['city'] = ddf.city
df['lattitude'] = ddf.lat
df['Longitude'] = ddf.lng
df['api_zip'] = ddf.zipcode
df.to_csv('Downloads/test_order.csv',index = False)"""
### This is to create a new file with the results
ddf.to_csv('Downloads/geo_info.csv',index = False)
\$\endgroup\$
5
  • \$\begingroup\$ Also, is ratelimiter the package available on PyPI (pypi.org/project/ratelimiter), or something else? \$\endgroup\$
    – Graipher
    Oct 11, 2018 at 16:02
  • \$\begingroup\$ @Graipher : hey, thanks ! :-) yes, while pasting it here it got off, i will correct it from next time. this works fine so was okay. :-). So while reviewing, do you take the code an run it also? Will surly take care of it. and yes, the package is same as you said. \$\endgroup\$ Oct 11, 2018 at 16:41
  • 1
    \$\begingroup\$ Being able to run the code usually improves the reviews (since you notice if you break something). But you don't need to include the API code (and probably shouldn't, since people could impersonate you then), reviewers can get by without it as well. On the other hand, the code actually working (on your setup) is of course a requirement here. Note that you can (and should in this case) edit your question. The only thing not allowed is modifying the code itself after having received answers (for that you would have to ask a new question). \$\endgroup\$
    – Graipher
    Oct 11, 2018 at 16:47
  • 1
    \$\begingroup\$ Thanks again @Graipher That helps a lot . I am gonna take care of all the points always, let me start with correcting the indentation first. \$\endgroup\$ Oct 11, 2018 at 16:56
  • 1
    \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$
    – Mast
    Oct 12, 2018 at 11:02

1 Answer 1

3
\$\begingroup\$

Let's start with the zipcode data imported from the CSV file. This should probably be contained in its own method:

def get_zipcodes(zip_code_col_name, file_name="Downloads/test_order.csv"):
    excel_data = pd.read_csv(file_name, encoding='utf8')
    if zip_code_col_name not in excel_data.columns:
        raise ValueError("Missing zipcode column")
    return excel_data[zip_code_col_name].unique().tolist()

Note that pandas.Series objects have a unique method that avoids your expensive loop (the in lookup is \$\mathcal{O}(n)\$ for lists). If that was not the case you could have used set(excel_data[zip_code_col_name].tolist()), instead. The tolist is not strictly needed, since a pandas.Series is also iterable (and that is all you do with it).

Your main code should also be in a function (called main, if you can think of nothing else):

def main():
    # Putting max call to 49 for 1 hour
    get_geo_info_limited = RateLimiter(max_calls=49, period=3600, callback=limited)(get_geo_info)

    results = [get_geo_info_limited(zipcode, API_KEY, RETURN_FULL_RESULTS) for zipcode in zipcodes]
    df = pd.DataFrame(results)
    df.to_csv('Downloads/geo_info.csv', index=False)

Here I used the fact that RateLimiter can also be used as a decorator (which is usually done using the @RateLimiter syntax during function definition, but can also be done manually like here) and a list comprehension instead of a manual for loop.

You can then call this main function under a if __name__ == "__main__": guard to enable importing from this module from another script.

\$\endgroup\$
9
  • \$\begingroup\$ could you please help me with this line please?- results = [get_geo_info_limited(zipcode, API_KEY, RETURN_FULL_RESULTS) for zipcode in zipcodes], how do i put the results of get_zipcodes function in there? \$\endgroup\$ Oct 12, 2018 at 10:41
  • \$\begingroup\$ Never mind , got it :-) \$\endgroup\$ Oct 12, 2018 at 10:48
  • \$\begingroup\$ Could we do something about api_key also? it should not be hard coded , right? \$\endgroup\$ Oct 12, 2018 at 10:53
  • 2
    \$\begingroup\$ @BlackCurrant You could pass it as an argument to the script. There are multiple methods to do that. For example, using argparse as demonstrated here. \$\endgroup\$
    – Mast
    Oct 12, 2018 at 11:04
  • 1
    \$\begingroup\$ thanks a lot here it is- codereview.stackexchange.com/questions/209099/… \$\endgroup\$ Dec 6, 2018 at 8:12

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.