8
\$\begingroup\$

This program will ask for user to insert summoner and base from the last 20 games of said summoner, it will give avg stats and see if good or not by wins in last 20 games(a simple grading system).

import requests
from getId import idcollect
from games import GAME
from wins import win_calc

#Key for riot API
Key = '**********************'

#ASKING USER FOR SUMMONER NAME
summonerName = input('Enter summoner name:')

#Objects
ids=idcollect()
game=GAME()
wins=win_calc()

#Collecting the acc id of summoner name
accId=ids.ID_collected(summonerName,Key)

#Collecting game id lists
game_list=[]
game_list=game.find_game_ids(accId,Key)

#Collecting wins list
win_list=[]
win_list=game.game_data(game_list,Key,summonerName)

#Calcuate whether the summoner is good or not
wins.is_dis_mane_good(win_list)
import requests


class GAME:


    def find_game_ids(self,accId,key):
        
        i=0
        GAMEID = []
        Idgame=20       
        url_match_list=('https://na1.api.riotgames.com/lol/match/v4/matchlists/by-account/'+(accId)+'?queue=420&endIndex=20&api_key='+(key))
        response2=requests.get(url_match_list)
        #Adding 20 games into the list
        while Idgame>0:
            GAMEID.append('https://na1.api.riotgames.com/lol/match/v4/matches/'+str(response2.json()['matches'][i]['gameId'])+'?api_key='+(key))
            i=i+1
            Idgame=Idgame-1

        return GAMEID


    def game_data(self,game_list,key,sumName):

        wins=[]
        deaths=[]
        deaths= []
        kills=[]
        assists=[]
        visions=[]
        csTotal=[]
        
        
        #Finding the data of said summoner in each game id
        for urls in game_list:

            
            response=requests.get(urls)
            Loop=0
            index=0
            while Loop<=10:
                
                if response.json()['participantIdentities'][index]['player']['summonerName']!=sumName:
                    Loop= Loop+1
                    index=index+1
                elif response.json()['participantIdentities'][index]['player']['summonerName']==sumName:
                    
                    deaths.append(response.json()['participants'][index]['stats']['deaths'])
                    kills.append(response.json()['participants'][index]['stats']['kills'])
                    assists.append(response.json()['participants'][index]['stats']['assists'])
                    visions.append(response.json()['participants'][index]['stats']['visionScore'])
                    csTotal.append(response.json()['participants'][index]['stats']['totalMinionsKilled'])
                    wins.append(response.json()['participants'][index]['stats']['win'])
                
                    break
        #Finding avg of each stat
        deaths=sum(deaths)/20            
        kills=sum(kills)/20
        assists=sum(assists)/20
        visions=sum(visions)/20
        csTotal=sum(csTotal)/20

        print('The avg kills is '+str(kills)+'\nThe avg deaths is '+str(deaths)+'\nThe avg assists is '+str(assists)+'\nThe avg visions is '+str(visions)+'\nThe avg cs total is '+str(csTotal))

        return wins
import requests

class idcollect:
    
    def ID_collected(self,sumName,key):
        #COLLECTING DATA TO BE INSERTING FOR MATCHLIST DATABASE
        url=('https://na1.api.riotgames.com/lol/summoner/v4/summoners/by-name/'+(sumName)+'?api_key='+(key))
        response=requests.get(url)
        accId=(response.json()['accountId'])
        return accId
import random

class win_calc:


    def is_dis_mane_good(self,winlist):
        
        winlist=sum(winlist)/20

        if (winlist<.33):
            trash=['DIS MANE STINKS','run while you can','I repeat, YOU ARE NOT WINNING THIS','I predict a fat L','Have fun trying to carry this person','He is a walking trash can','He needs to find a new game','BAD LUCK!!!']
            print (random.choice(trash))

        elif (winlist>.33 and winlist<=.5):
            notgood=['Losing a bit','Not very good','He needs lots of help','Your back might hurt a little','Does not win much'] 
            print (random.choice(notgood))

        elif (winlist>.5 and winlist<=.65):
            ight=['He is ight','He can win a lil','You guys have a decent chance to win','Serviceable','Should be a dub']
            print (random.choice(ight))

        elif (winlist>.65):
            good=['DUB!','You getting carried','His back gonna hurt a bit','winner winner chicken dinner','Dude wins TOO MUCH','You aint even gotta try','GODLIKE']
            print (random.choice(good))
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Welcome to Code Review. I've replaced the title with a more fitting description of your program. Feel free to edit your post to include your concerns into the post text; the title should simply state what your program is about, not your concerns. I hope you get some nice reviews :). \$\endgroup\$
    – Zeta
    Commented Aug 12, 2020 at 5:48
  • \$\begingroup\$ You forgot to mention that this is a revised version of codereview.stackexchange.com/questions/247554/…. Additionally, there are things you haven't done, like, making your code PEP8 compliant, which were refered in the other post. \$\endgroup\$ Commented Aug 12, 2020 at 15:07

1 Answer 1

13
\$\begingroup\$

find_game_ids is far more complicated than it needs to be. You have essentially two "counters", Idgame and i. One is being used to be placed in a string, and the other is to limit how many loops happen, but they're the same value if you think about it; just opposites. You don't need Idgame since you can just check if i < 20. You also don't need to manually manage i. range is for use-cases exactly like this:

def find_game_ids(self, accId, key):
    game_id = []
    url_match_list = f"https://na1.api.riotgames.com/lol/match/v4/matchlists/by-account/{accId}?queue=420&endIndex=20&api_key={key}"
    response2 = requests.get(url_match_list)
    for i in range(20):
        game_id.append(f"https://na1.api.riotgames.com/lol/match/v4/matches/{response2.json()['matches'][i]['gameId']}?api_key={key}"

    return game_id

i here will be every number from 0 to 19. I would also recommend creating a variable elsewhere to hold the 20 and call in N_GAMES or something. You seem to use that 20 in multiple spots. If you change it in one place and forget to change it somewhere else, you'll potentially have a nasty bug.

Other things I changed:

  • Variable names should be lowercase, separated by underscores according to PEP8. You have names all around this file that inconsistently use Upper_case. Use lower_case unless you're naming a class name.
  • Instead of adding string together using +, I changed it to use f-strings (note the f before the quotes). That lets you put a variable directly into a string using the {variable_name} syntax.

This can be further improved though. If you're iterating to create a list like you are here, list comprehensions can sometimes be cleaner:

def find_game_ids(self, accId, key):
    url_match_list = f"https://na1.api.riotgames.com/lol/match/v4/matchlists/by-account/{accId}?queue=420&endIndex=20&api_key={key}"
    response2 = requests.get(url_match_list)
    
    return [f"https://na1.api.riotgames.com/lol/match/v4/matches/{response2.json()['matches'][i]['gameId']}?api_key={key}"
            for i in range(20)]

The major readability problem in each case stems from how long that string is. You may want to break it over multiple lines, or generate it outside of the function using another function.


In game_data, you're calling response.json() repeatedly. Looking over the source of that method, it does not appear to do any caching. That means that every call to .json will reparse the data, which is a waste of CPU time. Save that into a variable once and use it as needed:

def game_data(self, game_list, key, sumName):
    . . .
    for urls in game_list:

        response = requests.get(urls)
        resp_json = response.json()  # Save it to use it again later
        Loop = 0
        index = 0
        while Loop <= 10:

            if resp_json['participantIdentities'][index]['player']['summonerName'] != sumName:
                Loop = Loop + 1
                index = index + 1
            elif resp_json['participantIdentities'][index]['player']['summonerName'] == sumName:

                deaths.append(resp_json['participants'][index]['stats']['deaths'])
                kills.append(resp_json['participants'][index]['stats']['kills'])
                assists.append(resp_json['participants'][index]['stats']['assists'])
                visions.append(resp_json['participants'][index]['stats']['visionScore'])
                csTotal.append(resp_json['participants'][index]['stats']['totalMinionsKilled'])
                wins.append(resp_json['participants'][index]['stats']['win'])

        . . .

Not only is that shorter, it also makes it easier to add in some preprocessing to the data later, and also has the potential to be much faster, because you aren't doing the same processing over and over again.


#Finding avg of each stat
deaths=sum(deaths)/20            
kills=sum(kills)/20
assists=sum(assists)/20
visions=sum(visions)/20
csTotal=sum(csTotal)/20

Like I said, you're using 20 in multiple places. What if you want to change this number later? It's not going to be fun to go around and find every relevant 20 and update it to the new value.

Have that number stored once, and use that variable:

# Top of file by imports
N_GAMES = 20

. . .

# The for-loop in the updated find_game_ids
for i in range(N_GAMES):

. . .

# At the bottom of game_data
deaths=sum(deaths)/N_GAMES            
kills=sum(kills)/N_GAMES
assists=sum(assists)/N_GAMES
visions=sum(visions)/N_GAMES
csTotal=sum(csTotal)/N_GAMES

For the classes win_calc and id_collect, there a few noteworthy things.

First, they shouldn't be classes. A good indicator that you shouldn't be using a class is that you're never using self in any of its methods. By using a class in this case, you need to construct an empty object just to call a method on it, which you're doing here:

wins=win_calc()

Just to call a method on it later:

wins.is_dis_mane_good(win_list)

Just make those classes plain functions:

import random

def is_dis_mane_good(winlist):

    winlist = sum(winlist) / 20

    if (winlist < .33):
        trash = ['DIS MANE STINKS', 'run while you can', 'I repeat, YOU ARE NOT WINNING THIS', 'I predict a fat L',
                 'Have fun trying to carry this person', 'He is a walking trash can', 'He needs to find a new game',
                 'BAD LUCK!!!']
        print(random.choice(trash))
    . . .

And then just use them as plain functions:

is_dis_mane_good(win_list)

Second, if it were appropriate to have them as classes, the names should be in CapitalCase: WinCalc and IDCollect (or maybe IdCollect).


Also, I'd rename is_dis_mane_good. Using a slang in the output of the program is one thing, but naming your methods obscure names isn't doing yourself or other readers of your code any favors.

As well in that function, I'd make some more changes:

  • I suggest you prefix your decimal numbers with a 0. 0.33 is much more readable than .33.

  • You can use operator chaining to simplify those checks too. winlist > 0.33 and winlist <= 0.5 can become 0.33 < winlist <= 0.5. As noted in the comments though, you can actually get rid of half of each check since, for example, if winlist < 0.33 was false, then you know winlist must be greater than 0.33, so the winlist > 0.33 check is redundant.

  • There's that 20 again ;). The more places you have it, the more likely you are to forget to update at least one of them. I'd use N_GAMES there instead.

  • You can get rid of the duplicated print(random.choice(. . .)) calls by assigning the list to a variable after each check, then having one print at the bottom.

After those changes, I'm left with this:

def competency_message(winlist):
    winlist = sum(winlist) / N_GAMES

    message_set = []
    if winlist < 0.33:  # Should be winlist <= 0.33 maybe?
        message_set = ['DIS MANE STINKS', 'run while you can', 'I repeat, YOU ARE NOT WINNING THIS', 'I predict a fat L',
                 'Have fun trying to carry this person', 'He is a walking trash can', 'He needs to find a new game',
                 'BAD LUCK!!!']

    elif winlist <= 0.5:
        message_set = ['Losing a bit', 'Not very good', 'He needs lots of help', 'Your back might hurt a little',
                   'Does not win much']

    elif winlist <= 0.65:
        message_set = ['He is ight', 'He can win a lil', 'You guys have a decent chance to win', 'Serviceable',
                'Should be a dub']

    else:
        message_set = ['DUB!', 'You getting carried', 'His back gonna hurt a bit', 'winner winner chicken dinner',
                'Dude wins TOO MUCH', 'You aint even gotta try', 'GODLIKE']

    print(random.choice(message_set))
\$\endgroup\$
4
  • 1
    \$\begingroup\$ Thx all these tips are really helpful. \$\endgroup\$ Commented Aug 12, 2020 at 1:36
  • \$\begingroup\$ Note that you can also remove a bit of duplication in that if/elif structure, by writing it as if winlist < 0.33: ... elif winlist <= 0.5: ... elif winlist <= 0.65: ... etc \$\endgroup\$
    – canton7
    Commented Aug 12, 2020 at 12:18
  • \$\begingroup\$ Ah, I meant e.g. elif 0.5 < winlist <= 0.65 could be elif winlist <= 0.65 \$\endgroup\$
    – canton7
    Commented Aug 12, 2020 at 13:39
  • 1
    \$\begingroup\$ @canton7 Whoops, sorry, just woke up. Updated and added some explanation. \$\endgroup\$ Commented Aug 12, 2020 at 13:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.