Team Fortress 2 item price lookup using Trade.tf developer API and Steam WebAPI

Question

I've made a nice little Python program using the requests module using the Trade.tf Developer API and the Steam WebAPI allowing the user to look up the price of Team Fortress 2 items through Trade.tf's spreadsheet.

For those of you that may not know, Team Fortress 2 is a free-to-play team-based shooter game which also features quite a large e-economy for in-game items. Sites such as Backpack.tf and Trade.tf exist to show pricing of items in terms of various currency-like items such as refined metal (a crafting ingredient in-game) and keys (used to open supply crates which can unlock various items within them).

I know there's definitely something that can be improved in this program, but I'm not fully sure on how I can accomplish this. Feel free to make any suggestions for it.

import json
import requests
import time

with open('keys.json', 'r') as f:
    keys = json.load(f)
    schema = requests.get('http://api.steampowered.com/IEconItems_440/GetSchema/v0001/?key=' + keys['steam'])
    spreadsheet = requests.get('http://www.trade.tf/api/spreadsheet.json?key=' + keys['tradetf'])

qualities = {"uncraft"    : -1,
             "genuine"    : 1,
             "vintage"    : 3,
             "unique"     : 6,
             "strange"    : 11,
             "haunted"    : 13} # all qualities currently tracked by Trade.TF

def get_item_defindex_from_name(name):
    for items in schema.json()["result"]["items"]:
        if items["name"] == name:
            defindex = items["defindex"]
    if defindex:
        return defindex
    else:
        print("Invalid item.")

def get_quality_id_from_name(name):
    return qualities[name]

def get_price_unit(unit):
    return {
    "r": " refined",
    "k": " keys",
    "b": " buds"}.get(unit, " ")

def get_tradetf_price(item, quality):
    price = spreadsheet.json()["items"][str(item)][str(quality)]["regular"]["hi"]
    unit  = get_price_unit(spreadsheet.json()["items"][str(item)][str(quality)]["regular"]["unit"])
    return str(price) + unit

def get_input(dialog, validation, error_message):
    while True:
        answer = input(dialog)
        if validation(answer):
            return answer
        else:
            print(error_message)

def main():
    try:
        while True:
            try:
                item_name = str(input("Enter item name: "))
                item_ = get_item_defindex_from_name(item_name)
                break
            except UnboundLocalError:
                print("Invalid item.")

        quality = get_input(
            "Enter item quality: ",
            lambda quality: quality.lower() in qualities,
            "Quality not tracked by Trade.tf."
        )
        quality_ = get_quality_id_from_name(quality.lower())

        print("\n", quality, item_name, "price: ", get_tradetf_price(item_, quality_))
    except KeyError:
        print("Item not currently tracked by Trade.tf.")

if __name__ == "__main__":
    main()
    print("Shutting down...")
    time.sleep(3)

Answer 1

Hah, this is a fun one cause I play a lot of Tf2. So let's see what could be improved here.

I'll start with you naming. You don't need to use the _from_ naming convention. ie.

def get_item_defindex_from_name(name):

get_item_defindex_from_name(item_name)

You already have the name there, it's clear that you need a name to get the defindex from, so remove that part and save yourself from a 5 word function name

get_item_defindex(item_name)

You should also use constants. qualities is essentially a constant but you haven't signalled it as one. To make it clear it is, write the name in UPPER_SNAKE_CASE (QUALITIES) and define it right after your imports, before running any actual code. I would do the same with your price units, it's not really clear why you treated them differently.

Your with block is a bit misplaced. If you definitely want to load the schema and spreadsheet as part of the global namespace to be accessed in every function, then you should still put them in the if __name__ == "__main__" block. if blocks don't limit scope the way functions do, so they'll still be globally available. You also never use the spreadsheet or the schema without calling .json() on them. Couldn't you just do this here? If you did you could probably save significant time not having to run that function every time. In fact, you only need items out of spreadsheet and ["result"]["items"] out of schema. If you don't need to maintain some sort of connection to the results (which it doesn't seem like you do) then you can just store the relevant values from these dicts and ignore the rest.

In get_item_defindex, you're looping over all the items to try find one with a matching name. Is there any reason to keep looking after you've found a value? I'm assuming not, so in that case you should return as soon as you find it to save time:

def get_item_defindex_from_name(name):
    for items in schema.json()["result"]["items"]:
        if items["name"] == name:
            return items["defindex"]
    print("Invalid item.")

If you specifically need to get the last matching name for some reason, then you should loop over the schema backwards:

def get_item_defindex_from_name(name):
    for items in reversed(schema.json()["result"]["items"]):
        if items["name"] == name:
            return items["defindex"]
    print("Invalid item.")

get_quality_id_from_name seems like a redundant function to me. If it's just going to be a dictionary lookup, it's easier to use that instead of a function. If it included getting the user input then it could be worth being a function. Often single lines of code are only worth being a function if they're particularly complex and worth abstracting or if they're called a lot and it saves repetitive code. Neither is true here. I'd say the same for price unit.

In get_tradetf_price you can make it readable and slightly more performant to grab the item up front, before you get the price and unit:

def get_tradetf_price(item, quality):
    item_data = spreadsheet.json()["items"][str(item)][str(quality)]["regular"]
    price = item_data["hi"]
    unit  = get_price_unit(item_data["unit"])
    return str(price) + unit

Your get_input function is very confusing. It's a good abstraction mostly, but the validation approach is overcomplicated, especially for what you need here. Instead of passing a lambda to test the input, just pass a list of valid values and test if the input is in there, like so:

def get_input(dialog, valid_inputs, error_message):
    while True:
        answer = input(dialog).lower()
        if answer in valid_inputs:
            return answer
        else:
            print(error_message)

You can then call it with a much simpler line:

    quality = get_input("Enter item quality: ", QUALITIES,
                        "Quality not tracked by Trade.tf.")

Also note I added the .lower() to when you call input. When accepting input try to sanitise it immediately. You don't care about the case so lowercase it as soon as you accept the input.

Note that if you can store the dictionary of items from the schema you can then reuse this for your item_name request at the start:

    item_name = get_input("Enter item name: ", schema_items,
                          "Invalid item.")

And lastly, your try block is a bit long. You clearly intend it to catch errors from get_tradetf_price but it's going to swallow any KeyError in your whole main block. Make try blocks as short as possible:

def main():
    item_name = get_input("Enter item name: ", schema_items,
                          "Invalid item.")
    item_id = get_item_defindex(item_name)

    quality = get_input("Enter item quality: ", QUALITIES,
                        "Quality not tracked by Trade.tf.")
    quality_id = get_quality_id_from_name(quality.lower())

    try:
        print("\n {} {} price: {}".format(quality, item_name,
                                          get_tradetf_price(item_id, quality_id))
    except KeyError:
        print("Item not currently tracked by Trade.tf.")

Answer 2

The only other thing I'd add is that instead of printing an error message and returning no value / None functions should be more explicit by throwing an exception in case None isn't actually handled later: At the moment the None return value from get_item_defindex will cause an error much later than where the actual "error" occurs. Better locality helps enormously with debugging since you don't have to backtrack through the code to look for where the "bad" value first occurred.