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))
1 Answer 1
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 paste
api_key=
and the url in so many places. Something likebuilder.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 andGAMEID
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 begame_url_list
, right? Without context it might seem that this is a list of Game objects. Also, try to keep consistent with variable styleLoop
vsresp_json
vscsTotal
. - 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
andelse
. - 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