1
\$\begingroup\$

This is my first ever python project finished from start to end. This is the third iteration after posting twice before and using the feed back. This time I decided to add tkinter. Just seeing if there code in my program that could have been done better. Also I lots of code uses self and init and I can never seem to understand it and when to use it. Also I don't really like using python = sys.executable os.execl(python, python, * sys.argv) Just wondering their a better way to reset program without actual restarting the program.

from tkinter import *
import os
import sys
import requests
import tkinter as tk
from getId import idcollect
from games import GAME
from wins import win_calc

# Variables
global Key
Key = '***********************************'
global num_game
num_game = 20
stat_list = [1, 2, 3, 4, 5, 6]
truth = ' '

# Making objects for other classes
ids = idcollect()
game = GAME()
wins = win_calc()

# Restart new
def restart_program():
    python = sys.executable
    os.execl(python, python, * sys.argv)

# Switching frames
def raise_frame(frame):
    frame.tkraise()

# Collecting the data from riot api
def collecting_data():
    name = entry_1.get()
    accId = ids.ID_collected(name, Key)
    
    if accId != 'NO':
        game_list = []
        game_list = game.find_game_ids(accId, Key, num_game)
        global stat_list
        stat_list = game.game_data(game_list, Key, name, num_game)
        global truth
        truth = wins.is_he_good(stat_list[5])
    
        label_kill.configure(text = stat_list[1])
        label_death.configure(text = stat_list[0])
        label_cs.configure(text = stat_list[4])
        label_honest.configure(text = truth)
    else:
        restart_program()
    


# Making main frame  
window = Tk()
window.geometry('500x500')
window.title("Riot Skills Evaluator")

# kill avg frame
kill_frame = Frame(window)
kill_frame.place(x = 0, y = 0, width = 500, height = 500)

# death avg frame
death_frame = Frame(window)
death_frame.place(x = 0, y = 0, width = 500, height = 500)

# cs avg frame
cs_frame = Frame(window)
cs_frame.place(x = 0, y = 0, width = 500, height = 500)

# being honest frame
honest_frame = Frame(window)
honest_frame.place(x = 0, y = 0, width = 500, height = 500)

# Multiple options frame
second_frame = Frame(window)
second_frame.place(x = 0, y = 0, width = 500, height = 500)

# Main menu 
first_frame = Frame(window)
first_frame.place(x = 0, y = 0, width = 500, height = 500)

# First frame widgets
label_0 = Label(first_frame, text = "Enter summoner name:", width = 20, font = ("bold", 20))
label_0.place(x=90,y=53)
entry_1 = Entry(first_frame)
entry_1.place(x=190,y=130)
Button(first_frame, text = 'Search', width = 20, bg = 'brown', fg = 'white', command = lambda:[raise_frame(second_frame), collecting_data()]).place(x=180,y=200)

# Second frame widgets
Button(second_frame, text = 'Kills average',width = 20, bg ='brown', fg = 'white', command = lambda:raise_frame(kill_frame)).place(x=180,y=100)
Button(second_frame, text = 'Death average',width = 20, bg ='brown', fg = 'white', command = lambda:raise_frame(death_frame)).place(x=180,y=150)
Button(second_frame, text = 'Cs average',width = 20, bg ='brown', fg = 'white', command = lambda:raise_frame(cs_frame)).place(x=180,y=200)
Button(second_frame, text = 'Honest truth',width = 20, bg = 'brown', fg = 'white', command = lambda:raise_frame(honest_frame)).place(x=180,y=250)
Button(second_frame, text = 'Back',width = 20,bg = 'brown', fg='white', command = restart_program).place(x=180,y=300)

# KillAvg frame
label_kill = Label(kill_frame, text = stat_list[1], width=20,font=("bold", 20))
label_kill.place(x=90,y=53)
Button(kill_frame, text = 'Back',width = 20,bg = 'brown', fg='white', command = lambda:raise_frame(second_frame)).place(x=180,y=300)

# DeathAvg frame
label_death = Label(death_frame, text = stat_list[0], width=20,font=("bold", 20))
label_death.place(x=90,y=53)
Button(death_frame, text = 'Back',width = 20,bg = 'brown', fg='white', command = lambda:raise_frame(second_frame)).place(x=180,y=300)

# Cs Average
label_cs = Label(cs_frame, text = stat_list[4], width=20,font=("bold", 20))
label_cs.place(x=90,y=53)
Button(cs_frame, text = 'Back',width = 20,bg = 'brown', fg='white', command = lambda:raise_frame(second_frame)).place(x=180,y=300)

# Honest Truth
label_honest = Label(honest_frame, text = truth, width=20,font=("bold", 20))
label_honest.place(x=90,y=53)
Button(honest_frame, text = 'Back',width = 20,bg = 'brown', fg='white', command = lambda:raise_frame(second_frame)).place(x=180,y=300)

window.mainloop()

games file

import requests


class GAME:

    def find_game_ids(self, accId, key, num_games):
        i = 0
        GAMEID = []
        num_games = 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 num_games > 0:
            GAMEID.append('https://na1.api.riotgames.com/lol/match/v4/matches/'+str(response2.json()['matches'][i]['gameId']) + '?api_key=' + (key))
            i = i + 1
            num_games = num_games - 1

        return GAMEID

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

        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)
            resp_json = response.json()
            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'])

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

        stat_list = []
        stat_list.append(deaths) #0
        stat_list.append(kills) #1
        stat_list.append(assists) #2
        stat_list.append(visions) #3
        stat_list.append(csTotal) #4
        stat_list.append(wins) #5
        return stat_list

getid file

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)
        if response.status_code == 200:
            accId = (response.json()['accountId'])
            return accId
        else: 
            accId = 'NO'
            return accId

wins file

import random


class win_calc:

    def is_he_good(self, winlist):


        if (winlist < 0.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!!!']
            return (random.choice(trash))

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

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

        elif (winlist > 0.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']
            return (random.choice(good))
\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

Note: below is just my opinion, do not treat it as a single source of truth. Also, I am assuming that the code works as it is expected, I will not mention functionality or any errors in logic.

General remarks:

  • I like that you have split the code to multiple files. One of my favorite points of python's zen is 'Namespaces are one honking great idea -- let's do more of those!'
  • Try to employ some linter and formatter to keep your code consistent, otherwise the code might be hard to read. I am personally using black and pylint, I would suggest to take a look.

First file

  • Do not use global variables as much as it is possible, do not treat them as a convinient way of storing state. This will cause a lot of problems in the future. For example, what if you want to run two identical things in parallel? They will have to share this state.
  • Do not use star imports (import *) it might cause names clash and is a bad practice.
  • os.execl(python, python, * sys.argv) as you mention, this is not great solution. The question here is WHY do you want to restart it? You should never have to restart the whole script. Instead, try to reset the state, if you remove the global variables and wrap the whole state in a couple of objects, it should be fairly easy.
  • Use if __name__ == "__main__" instead putting all the code directly into the script.
  • Extract different initializations to their own functions instead of adding comments.
  • There seems to be a lot of duplication, try to extract commonalities (e.g. button initialization).
  • Many buttons seem to have identical setup - you can extract those to a constant, then if you would like to resize it will be one change instead of many
  • In the first file you can keep the main initialization, but I would move out all things related to tinker to other file, wrap it with a class and import and create the window here.

Second file

Overall, I'd suggest to extract a RiotClient that would do the calls for you, this way you can extract the responsibility to other place. Also, currently the class is untestable (via unit tests) without 'hacking' the requests package

  • Extract the endpoint to a provider, this way the user can switch between different riot servers.
  • Use string interpolation instead of concatenation
  • You might want to do a builder for queries - this way you don't have to copy pasteapi_key= and the url in so many places. Something like builder.withServer("na1").withEndpoint("matchlist/by-account").withToken(token) or something similar.
  • find_game_ids can be split, currently it has two responsibilities - calling the API and
  • GAMEID should be lowercase, capital case is should be reserved for (global) constants (see here)
  • Try to use typing package, for example, game_list does not say anything about strings so it might be hard to figure out.
  • Try to name your variables by what they are storing, for example I think game_list should be game_url_list, right? Without context it might seem that this is a list of Game objects. Also, try to keep consistent with variable style Loop vs resp_json vs csTotal.
  • Try to name the functions by what they are doing, game_data does not indicate what this function does.
  • Consider adding error handling - what if API call fails for some reason (e.g. rate exceeded or some 5XX?). Such logic could be stored in before mentioned RiotClient.
  • Create a 'typed' return type from game_data, returning array will require the consumer to either know the internal implementation (which field goes to whcih array element) or negotiate a contract which is impractical if you can just create an object with names for properties.
  • Try to split your functions into multiple smaller ones and name them by what they are doing. For example, whenever you feel that you want to write a comment (# Finding avg of each stat), extract it to a function (get_avg_of_stats).

Third file

  • Class name should be CamelCase
  • Function name should be lower_case
  • Here you have error handling - very good. This could be still abstracted to RiotClient though.
  • You can have a single return - no need to have it for both if and else.
  • I like that the file is small and has only one class which has a single responsibiliy - this is a pattern that you should apply to other functionalities. However, I don't think this should be a class - maybe just a function? You never use self.

Fourth file

  • Class name should be CamelCase
  • Maybe better name for the method would be is_player_good ?
  • For if statements you can use chaining syntax - if 0.33 < winlist <= 0.5
  • No need for parenthesis in if, elif, return statements
  • You could have a single return here but I personally wouldn't mind the current solution
  • This should be a function, not a class
\$\endgroup\$

Your Answer

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

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