Star Wars API GET request in Python

Question

I am working with a Star Wars API from http://swapi.co/api/. Here's the problem that I was working on for the Star Wars API problem. And the code works and prints out the exact output that I desired. I would like to get some code review.

>

# You are a Coordinator for the Star Wars rebel forces and are responsible for planning troop deployments.
# You need a tool to help find starships with enough passenger capacity and pilots to fly them.



# Build a CLI tool that takes as its single argument the number of people that
# need to be transported and outputs a list of candidate pilot and starship names
# that are each a valid possibility.
# Assume that any pilot can be a candidate regardless of allegiance.
# Assume the entire group of passengers must fit in a single starship.

# You may skip starships with no pilots or with unknown passenger capacity.
# Your tool must use the Star Wars API (http://swapi.co) to obtain the necessary data.

# You may not use any of the official Star Wars API helper libraries but can use any other libraries you want
# (http client, rest client, json).

Example:

> # print-pilots-and-ships with minimum of 20 passengers
>
> # Luke Skywalker, Imperial shuttle
>
> # Chewbacca, Imperial shuttle
>
> # Han Solo, Imperial shuttle
>
> # Obi-Wan Kenobi, Trade Federation cruiser
>
> # Anakin Skywalker, Trade Federation cruiser

Python 3 solution:

import sys
import requests
import json
import urllib.parse

#number of pages in JSON feed

def print_page(page_num, num_passenger):
    endpoint = "https://swapi.co/api/starships/?"
    type = 'json'

    #specifies api parameters
    url = endpoint + urllib.parse.urlencode({"format": type, "page": page_num})

    #gets info
    json_data = requests.get(url).json()
    # number_of_ship = json_data['count']
    if 'results' in json_data:
      for ship in json_data['results']:
          if has_pilot(ship) and has_enough_passenger(ship, num_passenger):
              print_pilots_on(ship)

def get_pilot_name(pilot):
    type = 'json'

    #specifies api parameters
    url = pilot

    #gets info
    json_data = requests.get(url).json()
    return json_data["name"]

def print_pilots_on(ship):
    for pilot in ship['pilots']:
       print(get_pilot_name(pilot), ship['name'])

def has_pilot(ship):
    if ship['pilots']:
      return True
    return False

def has_enough_passenger(ship, num):
    if ship['passengers'] != "unknown" and int(ship['passengers']) >= num:
      return True
    return False

def print_pilots_and_ships(num_passenger):

    page_list = [1,2,3,4,5,6,7,8,9]
    # list to store names

    for page in page_list:
        print_page(page, num_passenger)


if __name__ == '__main__':
    num_passenger = int(20)
    print_pilots_and_ships(num_passenger)

Answer 1

This is pretty good. I only have a few small nitpicks

• Return directly

  You do this a few times:

   if ship['passengers'] != "unknown" and int(ship['passengers']) >= num:
      return True
  return False
  

  The above can be rewritten as:

  return ship['passengers'] != "unknown" and int(ship['passengers']) >= num
  

• Superfluous int call

  int(20), here the int call does nothing, 20 is already an integer.

• You can get more out of the request module.

  Python's requests lib is very powerful and has a lot of features you can use.

  It supports adding params for instance, so instead of:

  #specifies api parameters
  url = endpoint + urllib.parse.urlencode({"format": type, "page": page_num})
  #gets info
  json_data = requests.get(url).json()
  

  You could do:

  json_data = requests.get(endpoint, params={"format": "json", "page": page_num}).json()
  

• Remove unused imports

  sys module is not used, I suggest removing it.

Answer 2

In addition to Ludisposed's great suggestions, I'd recommend another kind of approach in the flow of code.

Right now, your call stack becomes main()->print_pilots_and_ships()->print_page()->print_pilots_on()->get_pilot_name(), all of them called within each other. This means that you can only ever get the final result and even that's printing.

It makes code like this difficult to test, maintain and reuse.

Instead, you could first separate functions that print from functions that return.

import requests
import json
import urllib.parse

PASSENGER_THRESHOLD = 20

def fetch_pilot(pilot_url):
    return requests.get(pilot_url).json()

def fetch_ships():
    endpoint = 'https://swapi.co/api/starships/?'

    ships = []
    for page_num in range(1, 10):
        params = urllib.parse.urlencode({'format': 'json', 'page': page_num})
        response = requests.get(endpoint + params).json()
        if 'results' in response:
            ships += response['results']

    return ships

def has_pilot(ship):
    return 'pilots' in ship

def has_enough_capacity(ship, threshold):
    return ship['passengers'] != 'unknown' and int(ship['passengers']) >= threshold

def is_eligible(ship):
    return has_pilot(ship) and has_enough_capacity(ship, PASSENGER_THRESHOLD)

for pilot_url in ship['pilots']:
        pilot = fetch_pilot(pilot_url)
        print(pilot, ship['name'])

if __name__ == '__main__':
    ships = fetch_ships()
    eligible_ships = [ship for ship in ships if is_eligible(ship)]

    for ship in eligible_ships:
        print_pilots(ship)

Here I have done a couple of things: I've called all functions that do API calls fetch_ to indicate that they do some heavier operations. I've followed Ludisposed's examples and made our is_ and has_ functions neater.

I did a minor change here as I save all the ships in a list and then iterate over that list when filtering. Since the amount of ships here is small, I felt comfortable doing that but if the data is big, it's gonna blow up so you can keep the filtering inside the fetch method as well.

Also in fetch_pilot, we fetch the entire model and then extract what we need when we need it. Now it's more reusable whenever in our code we want to retrieve pilot information.

I also renamed has_enough_passenger to has_enough_capacity since we're looking for the passenger capacity, not the amount of passengers.

Last, I refactored PASSENGER_THRESHOLD into a constant just to show that it's an option. Of course, in many scripts like these, it comes as a user input, in which case it's not really meaningful extraction.