4
\$\begingroup\$

I have written a program which will tell you how long it is until the next bus on “BUS ROUTE” leaving from “BUS STOP NAME” going “DIRECTION” using the api defined at http://svc.metrotransit.org/.

Example:

$ metro.py "metro blue line" "target field station platform 1" "south" 
6 minutes

I am very new to Python and wondering how this code could be better — more efficient, follow best practices, etc. What would you do differently?

import requests, sys, time

# need to have each: route, stop & direction
if len(sys.argv) != 4:
    sys.exit("Please be sure to enter a route, stop, and direction.")

route = sys.argv[1]
stop = sys.argv[2]
direction = sys.argv[3]

url = "http://svc.metrotransit.org/NexTrip/"

def getData (uri, key, value, arg):
    resp = requests.get(url + uri + "?format=json")
    data = resp.json()

    # default in case of invalid input 
    dataID = -1

    while True: 
        try: sys.exit(uri + str(resp.status_code))

        except:
            for item in data:
                if item[key].lower().find(arg) > -1:
                    dataID = item[value]
                    break
            return dataID

routeID = getData("Routes", "Description", "Route", route)

if routeID < 0:
    sys.exit(route + " is not a valid route.")

uri = routeID
directionID =  getData("Directions/" + uri, "Text", "Value", direction)

if directionID < 0:
    sys.exit(route + " does not go " + direction + ".")

uri += "/" + directionID 
stopID = getData("Stops/" + uri, "Text", "Value", stop)

if stopID < 0:
    sys.exit(stop + " is not along " + route + " going " + direction + ".")

uri += "/" + stopID
timeID = getData(uri, "RouteDirection", "DepartureTime", direction)

if timeID != -1:
    #Get 10 digit timestamp from response, - from current time, and / by 60 to get minutes as an integer
    time = int((float(timeID[6:16]) - time.time()) // 60)
    if time > 1:
        print str(time) + " minutes"
    else:
        print "1 minute or less"
\$\endgroup\$
4
\$\begingroup\$

As Ludisposed said in his answer, yours is a very top-down approach. The minimum you should do is put this into a function, so you can re-use it. Here I rewrote your code quite significantly. Instead of returning -1, use the fact that a Python function returns None by default, which is a better safe value than -1.

I also completely re-wrote your getData function. Honestly, I have no idea what half of that was trying to do. All you need to do is iterate over the data. No need for a try: sys.exit(...) except: (which will always call what is inside the except, so why bother?). Also no need for a while True loop, if you are only ever going to go through it once.

I added the simple argparse parser and added the calling code under a if __name__ == "__main__": guard.

import argparse
import time
import sys
import requests

URL = "http://svc.metrotransit.org/NexTrip/{}?format=json"


def get(path, key, value, arg):
    resp = requests.get(URL.format(path))
    data = resp.json()
    for item in data:
        if arg in item[key].lower():
            return item[value]


def next_bus(route, stop, direction):
    route_id = get("Routes", "Description", "Route", route)
    if route_id is None:
        raise RuntimeError("{} is not a valid route.".format(route))

    direction_id = get("Directions/{}".format(route_id),
                           "Text", "Value", direction)
    if direction_id is None:
        raise RuntimeError("{} does not go {}.".format(route, direction))

    stop_id = get("Stops/{}/{}".format(route_id, direction_id), "Text", "Value", stop)
    if stop_id is None:
        raise RuntimeError("{} is not along {} going {}.".format(
            stop, route, direction))

    time_ = get("{}/{}/{}".format(route_id, direction_id, stop_id),
                    "RouteDirection", "DepartureTime", direction)
    if time_ is None:
        raise RuntimeError("Could not get next connection.")
    return int((float(time_[6:16]) - time.time()) // 60)


if __name__ == "__main__":
    parser = argparse.ArgumentParser()
    parser.add_argument("route")
    parser.add_argument("stop")
    parser.add_argument("direction")
    args = parser.parse_args()
    time = next_bus(**vars(args))
    if time > 1:
        print "{} minutes".format(time)
    else:
        print "1 minute or less"

Alternatively, you could make this a class and use property for the different ids. This would allow you to have an object like trip_home = Connection(route, direction, stop), on which you could call trip_home.next to get the latest connection.

The different ids are looked up upon the initialization of the class. Calls to trip_home.next will only need to get the next connection. This assumes that the ids do not change during the runtime of the program, of course.

import argparse
import time
import sys
import requests


class Connection:
    url = "http://svc.metrotransit.org/NexTrip/{}?format=json"

    def __init__(self, route, stop, direction):
        self.route = route
        self.stop = stop
        self.direction = direction
        self.route_id = self.get_route_id()
        self.direction_id = self.get_direction_id()
        self.stop_id = self.get_stop_id()

    def __str__(self):
        return "{self.route} from {self.stop} due {self.direction}. Next connection in {self.next} minutes.".format(self=self)

    def get(self, path, key, value, arg):
        resp = requests.get(self.url.format(path))
        data = resp.json()
        for item in data:
            if arg in item[key].lower():
                return item[value]

    def get_route_id(self):
        id_ = self.get("Routes", "Description", "Route", self.route)
        if id_ is None:
            raise RuntimeError(
                "{self.route} is not a valid route.".format(self=self))
        return id_

    def get_direction_id(self):
        id_ = self.get(
            "Directions/{self.route_id}".format(self=self), "Text", "Value", self.direction)
        if id_ is None:
            raise RuntimeError("{self.route} does not go {self.direction}.".format(
                self=self))
        return id_

    def get_stop_id(self):
        id_ = self.get(
            "Stops/{self.route_id}/{self.direction_id}".format(self=self), "Text", "Value", self.stop)
        if id_ is None:
            raise RuntimeError(
                "{self.stop} is not along {self.route} going {self.direction}.".format(self=self))
        return id_

    @property
    def next(self):
        time_ = self.get("{self.route_id}/{self.direction_id}/{self.stop_id}".format(self=self),
                         "RouteDirection", "DepartureTime", self.direction)
        if time_ is None:
            raise RuntimeError(
                "Could not find next connection on route {self.route} from {self.stop} due {self.direction}".format(self=self))
        return int((float(time_[6:16]) - time.time()) // 60)


if __name__ == "__main__":
    parser = argparse.ArgumentParser()
    parser.add_argument("route")
    parser.add_argument("stop")
    parser.add_argument("direction")
    args = parser.parse_args()
    trip_home = Connection(args.route, args.stop, args.direction)
    print trip_home
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Why not change the print statement to a function for Python 3 compatibility? \$\endgroup\$ – Daniel Oct 11 '17 at 15:55
  • \$\begingroup\$ @Coal_ Because the OP used Python 2 print. But you are right, in this case there would be no difference when using it as if it was alreayd Python 3. \$\endgroup\$ – Graipher Oct 11 '17 at 15:57
  • \$\begingroup\$ @Graipher thanks so much, this is very informative and makes a lot of sense! \$\endgroup\$ – kat-gie Oct 11 '17 at 18:01
1
\$\begingroup\$

PEP8 and Readability

When coding it is nice to uphold a standard, it makes your code more readable for others and yourself. In python we have PEP8. Your code has a few PEP8 violations and other readability issues.

  • Multiple imports on one line, imports should each have thier own line.
  • Use a if __name__ == '__main__': block
  • globals should be uppercase URL = 'http://my_url.com'
  • Use argparse over sys.argv, much easier to handle arguments that way
  • Functions and variables should be snake_case instead of pascalCase
  • Use string.format() instead of string concatinations

Other

  • Your code has a Top-Down structure, I would refractor this into smaller pieces.
  • sys.exit(0) would be better (which means the program closed normally), and then print '{0} is not a valid route'.format(route)
| 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.