14
\$\begingroup\$

I wrote a script to fetch the location for the ISS using two simple APIs.

This is my first time writing a python script. Any advice or modifications would be highly appreciated. How is the code, and where are the areas I can improve upon?

This code is also on GitHub.

import urllib.request # to make requests to the api
import json # to parse the json response
import reverse_geocoder as rg # to get address from location

url = "http://api.open-notify.org/astros.json"
response = urllib.request.urlopen(url) # request the api, returns a JSON object
json_result = json.loads(response.read()) # read the JSON object

# Separate different values based on keys.
people = json_result['people'] # people currently in space
number_in_space = json_result['number'] # total people in space
print("People in space : ", number_in_space)
print("----NAMES----")

for p in people:
    print(p['name'])

# ISS data
iss_url = "http://api.open-notify.org/iss-now.json"
iss_response = urllib.request.urlopen(iss_url)
iss_json_result = json.loads(iss_response.read())
# Store the positions
latitude = iss_json_result['iss_position']['latitude'] # store the latitude
longitude = iss_json_result['iss_position']['longitude']
print("Latitude: ", latitude," -- ", "Longitude: ", longitude)

address = rg.search((latitude, longitude)) # Get the address from location tuple returns a list
address_name = address[0]['name']
address_admin1 = address[0]['admin1']
address_admin2 = address[0]['admin2']
address_cc = address[0]['cc']
print("----Address----")
print(address_name, ", ", address_admin1, address_admin2, address_cc)
\$\endgroup\$
12
\$\begingroup\$

Not bad for a first Python script!

In general, you can greatly benefit from giving parts of your code names, by encapsulating them in functions. This makes it also re-usable and let's you add a docstring giving more detail on what this function does.

If you add a main function which you call under a if __name__ == "__main__": guard you can also import these functions from another script without all of your code running.

Instead of urllib.request, you can use the requests module, it even has a method to directly return a JSON object.

While it is nice that the astronauts response contains the number of astronauts in space, you could get the same information from calling len(people), which is \$\mathcal{O}(1)\$ for Python lists.

Familiarize yourself with f-strings (Python 3.6+). They are a nice and compact way to write formatted strings, since they can contain arbitrary Python expressions (I used it for the address here).

import requests
import reverse_geocoder

def get_json(url):
    """Retrieve JSON object from url.

    Raises an exception if connection fails.
    """
    response = requests.get(url)
    response.raise_for_status()  # make sure an exception is raised if something goes wrong
    return response.json()

def get_astronauts():
    """Returns a list of all people currently in space"""
    url = "http://api.open-notify.org/astros.json"
    return get_json(url)['people']

def get_iss_location():
    """Returns the current latitude and longitude of the ISS"""
    url = "http://api.open-notify.org/iss-now.json"
    position = get_json(url)['iss_position']
    return position['latitude'], position['longitude']

def get_address(position):
    """Do a reverse lookup getting the closest address to a given position"""
    return reverse_geocoder.search(position)[0]

def main():
    astronauts_in_space = get_astronauts()
    print("People in space : ", len(astronauts_in_space))
    print("----NAMES----")
    for astronaut in astronauts_in_space:
        print(astronaut['name'])

    iss_position = get_iss_location()
    print("Latitude: {}, Longitude: {}".format(*position))

    address = get_address(iss_position)
    print("----Address----")
    print(f"{address['name']}, {address['admin1']} {address['admin2']} {address['cc']}")

if __name__ == "__main__":
    main()
\$\endgroup\$
  • 2
    \$\begingroup\$ I would add a function "get_data(url)" where you can pass the url and it returns a response \$\endgroup\$ – Margon Mar 19 at 13:54
  • 1
    \$\begingroup\$ @Margon True, I was on edge whether or not I would add it, since it is only repeated once... \$\endgroup\$ – Graipher Mar 19 at 13:55
  • 1
    \$\begingroup\$ @Margon This even fixed some bugs (typos and the fact that the dictionary returned by .json() does not have a raise_for_status method). \$\endgroup\$ – Graipher Mar 19 at 14:00
  • 3
    \$\begingroup\$ Yeah, I quite agree with you, but it's better to give the "best practices", so he can re-use the code if he wants to add more api \$\endgroup\$ – Margon Mar 19 at 14:00
  • \$\begingroup\$ Thank You very much :) \$\endgroup\$ – dsaharia Jun 25 at 7:42

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.