JSON Parser, pulls data from a JSON file for analysis

Question

This is the first time I have programmed something that I will actually personally use, so I am happy about that. I tried to write good code whilst applying functional programming concepts. So no state, and a concise function.

I know that when making the lists it pulls information from the JSON that doesn't get used later in the program. The data will be used once I get continue to develop the program further.

At the end there is a dictionary containing hero ID keys and name values. I cut out most of it to minimize the length of this post.

import json
import requests
def parsestats(herodata):
    return [(hero['heroId'], 
    hero['pickBan']['pick']['matchCount']/ # Divide by next line
        herodata['matchPickCount']*1000,
    hero['pickBan']['ban']['matchCount']/  # Divide by next line
        herodata['matchBanCount']*1000,
    hero['pickBan']['pick']['wins']*100, 
    hero['pickBan']['ban']['wins']*100,
    hero['pickBan']['pickLastWeek']['matchCount']/ # Divide by next line
        herodata['matchLastWeekPickCount']*1000,
    hero['pickBan']['banLastWeek']['matchCount']/  # Divide by next line
        herodata['matchLastWeekBanCount']*1000,
    hero['pickBan']['pickLastWeek']['wins']*100, 
    hero['pickBan']['banLastWeek']['wins']*100)
            for hero in herodata['heroes']]
print(f"{'Hero,':<20}{'Value':>7}")
for hero in sorted(
    [(uni[0], imm[1]*((uni[3] if uni[3] > imm[3] else imm[3])-50))
    for uni, imm in zip(  
        sorted(parsestats(json.loads(requests.get(
        'https://api.stratz.com/api/v1/Hero/directory/simple').text))), 
        sorted(parsestats(json.loads(requests.get(
        'https://api.stratz.com/api/v1/Hero/directory/simple?rank=8').text))))
    ],
    key=lambda tup: tup[1], reverse = True):
    print("{:<20}{:>7.2f}".format({1: 'Anti-Mage', 2: 'Axe', 3: 'Bane', 4: 'Bloodseeker', 5: 'Crystal Maiden', 6: 'Drow Ranger', 7: 'Earthshaker', 8: 'Juggernaut', 9: 'Mirana', 10: 'Morphling', 11: 'Shadow Fiend', 12: 'Phantom Lancer', 13: 'Puck', 14: 'Pudge', 15: 'Razor', 16: 'Sand King', 17: 'Storm Spirit', 18: 'Sven', 19: 'Tiny', 20: 'Vengeful Spirit', 21: 'Windranger', 22: 'Zeus', 23: 'Kunkka', 25: 'Lina', 26: 'Lion', 27: 'Shadow Shaman', 28: 'Slardar', 29: 'Tidehunter', 30: 'Witch Doctor', 31: 'Lich', 32: 'Riki', 33: 'Enigma', 34: 'Tinker', 35: 'Sniper', 36: 'Necrophos', 37: 'Warlock', 38: 'Beastmaster',39: 'Queen of Pain', 40: 'Venomancer', 41: 'Faceless Void', 42: 'Wraith King', 43: 'Death Prophet', 44: 'Phantom Assassin', 45: 'Pugna', 46: 'Templar Assassin', 47: 'Viper', 48: 'Luna', 49: 'Dragon Knight', 50: 'Dazzle', 51: 'Clockwerk', 52: 'Leshrac', 53: 'Nature\'s Prophet', 54: 'Lifestealer', 55: 'Dark Seer', 56: 'Clinkz', 57: 'Omniknight', 58: 'Enchantress', 59: 'Huskar', 60: 'Night Stalker', 61: 'Broodmother', 62: 'Bounty Hunter', 63: 'Weaver', 64: 'Jakiro', 65: 'Batrider', 66: 'Chen', 67: 'Spectre', 68: 'Ancient Apparition', 69: 'Doom', 70: 'Ursa', 71: 'Spirit Breaker', 72: 'Gyrocopter', 73: 'Alchemist', 74: 'Invoker', 75: 'Silencer', 76: 'Outworld Devourer', 77: 'Lycan', 78: 'Brewmaster', 79: 'Shadow Demon', 80: 'Lone Druid', 81: 'Chaos Knight', 82: 'Meepo', 83: 'Treant Protector', 84: 'Ogre Magi', 85: 'Undying', 86: 'Rubick', 87: 'Disruptor', 88: 'Nyx Assassin', 89: 'Naga Siren', 90: 'Keeper of the Light', 91: 'Io', 92: 'Visage', 93: 'Slark', 94: 'Medusa', 95: 'Troll Warlord', 96: 'Centaur Warrunner', 97: 'Magnus', 98: 'Timbersaw', 99: 'Bristleback', 100: 'Tusk', 101: 'Skywrath Mage', 102: 'Abaddon', 103: 'Elder Titan', 104: 'Legion Commander', 105: 'Techies', 106: 'Ember Spirit', 107: 'Earth Spirit', 108: 'Underlord', 109: 'Terrorblade', 110: 'Phoenix', 111: 'Oracle', 112: 'Winter Wyvern', 113: 'Arc Warden', 114: 'Monkey King', 119: 'Dark Willow', 120: 'Pangolier', 
        121: 'Grimstroke', 129: 'Mars'}[hero[0]], hero[1]))

Answer 1

What a wall of text! Also known as write-only code, or job security.

You want to write understandable code, that you can come back to in 6 months, and within 5 minutes understand enough to change it, if necessary. This has a for-loop over a sorted list comprehension, with embedded if-then-else, of a zip of two sorted json queries, with a lambda thrown in to influence sorting. This isn't functional programming; it's dysfunctional programming.

---

Let me begin with parsestats, because I can at least begin to understand it.

First, the name parsestats is itself a wall of characters. How do I pronounce that word? "par-sest-ats"? Oh! "parse stats"! Write function names with snake_case, to aid the reader find the words in your compound name. So this function should be called parse_stats.

In this function, you repeat hero['pickBan'] 8 times. This is a lot of repetition. It means the Python interpreter is looking up the same element in a dictionary repeatedly, which means the code is not going to win any speed contests. Look up the value once, and reuse that value, instead of relooking it up over and over again. This is awkward to do in a list comprehension, so lets use a helper function:

def parse_stats(herodata):

    def stats(hero_id, pick_ban):
        # ... TBD ... 

    return [ stats(hero['heroId'], hero['pickBan']) for hero in herodata['heroes']]

A 14-line list comprehension just got turned into a 1 line list comprehension, and hero['pickBan'] got stored in the function argument pick_ban, for reuse multiple times. I'm liking the direction this is going.

Now, what stats are being collected? Looks like you've got 8 stats: 4 from this week, and the same 4 from the previous week. Maybe we could compute those 4 stats in a function, passing in different selectors to grab the correct data?

def parse_stats(herodata):

    def stats(hero_id, pick_ban):

        def week_stats(pick, ban, pick_count, ban_count):

            return pick_ban[pick]['matchCount'] / pick_count * 1000,
                   pick_ban[ban]['matchCount'] / ban_count * 1000,
                   pick_ban[pick]['wins'] * 100,
                   pick_ban[ban]['wins'] * 100

        this_week = week_stats('pick', 'ban', this_week_pick, this_week_ban)
        last_week = week_stats('pickLastWeek', 'banLastWeek', last_week_pick, last_week_ban)

        return hero_id, *this_week, *last_week

    this_week_pick = herodata['matchPickCount']
    this_week_ban = herodata['matchBanCount']
    last_week_pick = herodata['matchLastWeekPickCount']
    last_week_ban = herodata['matchLastWeekBanCount']

    return [ stats(hero['heroId'], hero['pickBan']) for hero in herodata['heroes']]

Is that the clearest way? Maybe we want to use a when selector, instead:

def parse_stats(herodata):

    def stats(hero_id, pick_ban):

        def stats_for(when):

            pick = pick_ban[f'pick{when}']
            ban = pick_ban[f'ban{when}']

            return pick['matchCount'] / herodata[f'match{when}PickCount'] * 1000,
                   ban['matchCount'] / herodata[f'match{when}BanCount'] * 1000,
                   pick['wins'] * 100,
                   ban['wins'] * 100

        this_week = stats_for('')
        last_week = stats_for('LastWeek')

        return hero_id, *this_week, *last_week

    return [ stats(hero['heroId'], hero['pickBan']) for hero in herodata['heroes']]

Which would make it easier to select 'LastMonth' stats, too.

---

Is a tuple really the best data structure for parse_stats to return? The 0th element is the hero id. What is the 2nd element? Which element is last week's pick win percentage? What is [1], or [3]?

How about using a namedtuple?

from collections import namedtuple

Stats = named tuple('Stats',
                    ('id',
                     'pick_rate', 'ban_rate', 'pick_wins', 'ban_wins',
                     'pick_rate_last_week', 'ban_rate_last_week', 'pick_wins_last_week', 'ban_wins_last_week'))

# ...

def parse_stats(herodata):
    def stats(hero_id, pick_ban):
        # ...
        return Stats(hero_id, *this_week, *last_week)

Or maybe make a tuple of tuples.

Stats = namedtuple('Stats', 'id, this_week, last_week')
WeekStats = namedtuple('WeekStats', 'pick_rate, ban_rate, pick_wins, ban_wins')

def parse_stats(herodata):
    def stats(hero_id, pick_ban):
        # ...
        return Stats(hero_id, WeekStats(*this_week), WeekStats(*last_week))

And you can refer to uni.id and imm.this_week.pick_rate instead of uni[0] and imm[1].

---

Ok, the elephant in the program:

print(f"{'Hero,':<20}{'Value':>7}")
for hero in sorted(ridiculous_list_comprehension, key=lambda tup: tup[1], reverse = True):
    print("{:<20}{:>7.2f}".format(inline_dictionary_definition[hero[0]], hero[1]))

First off, this should be a function, not code executed when Python parses the program text. Use a if __name__ == '__main__': guard call the function if you want the code executed immediately upon parsing:

def print_mashup():
    print(f"{'Hero,':<20}{'Value':>7}")
    for hero in sorted(ridiculous_list_comprehension, key=lambda tup: tup[1], reverse = True):
        print("{:<20}{:>7.2f}".format(inline_dictionary_definition[hero[0]], hero[1]))

if __name__ == '__main__':
    print_mashup()

Of course, name the function according to what it really is doing.

for hero in sorted(...):

Uhm, no. The output of sorted() isn't a list of hero objects. You parsed all that hero data in parse_stats, and this is a much condensed summary of two values of those statistics, combined from two different data sets. It is a list of two-value tuples. Give those individual values names:

for hero_id, score in sorted(...):

I'm guess the "value" is some kind of score. Maybe rating is a better name. At any rate, this is way more descriptive than the hero[0] and hero[1] references made in the print statement of the body of the loop.

        print("{:<20}{:>7.2f}".format(inline_dictionary_definition[hero[0]], hero[1]))

Is that inline_dictionary_definition being constructed each and every iteration of the body of the for loop, so that one value can be extracted from the dictionary? Be kind. Create it once.

def print_mashup():
    print(f"{'Hero,':<20}{'Value':>7}")

    HERO_BY_ID = {1: 'Anti-Mage', 2: 'Axe', ... 129: 'Mars'}

    for hero_id, score in sorted(ridiculous_list_comprehension, key=lambda tup: tup[1], reverse = True):
        print(f"{HERO_BY_ID[hero_id]:<20}{score:>7.2f}")

As for the ridiculous_list_comprehension, I haven't a clue what "simple" is compared to "simple?rank=8", or what the calculated value is supposed to mean. Or what uni and imm are supposed to be abbreviations for. There are all sorts of unknowns. Do the json queries return exactly the same hero_id sets? Can there be an extra one in either of the sets, in which case sorting and zipping doesn't guarantee the data matches up properly!

I'm happy that you're happy to have written a program that you will personally use. I'd be happier if someone else could understand it, and use it too.