I don't understand what your code is trying to do with data
vs. data_list
. I rewrote it to get rid of data
. Also, after you compute efficiency
you append it to the list, but then you seem to be pulling it off the list again as score
. I simply got rid of that.
For working with files, best practice is to use a with
statement so I rewrote to do that.
Also, you are converting string items to integer one at a time, when you could do them all in one go with a list comprehension. I hope list comprehensions are not a problem for you to use, because they make the code cleaner. The first list comprehension calls the .strip()
method function on each of the three strings for names. The second one converts all the integers in one convenient way.
Instead of making a temp list named a
and then reversing that list, I just specified the reverse=True
option in sorted()
. Now the list is built in reverse order, just what you want.
As others have noted, your print function needs a loop to print things in the list. Since the message in the print function says it prints the top 50, I changed the list slicing to happen inside the print function. Now the print function takes an optional argument specifying how many items to print; it has a default value of 50, so if you don't specify another value, it prints the top 50 items.
While you don't need to do it, there is a common Python feature of putting if __name__ == __main__:
before your code. You can see an explanation here: What does if __name__ == "__main__": do?
def get_data_list (file_name):
player_list=[]
with open(file_name, "r") as f:
for line in f:
# split line on commas, and convert items to integer values
# make a list of the integer values.
items = line.split(',')
first_name = items[2].strip()
last_name = items[3].strip()
team_name = items[4].strip()
data_list = [int(x) for x in items[6:]
gp = data_list[0]
mins = data_list[1]
pts = data_list[2]
oreb = data_list[3]
dreb = data_list[4]
reb = data_list[5]
asts = data_list[6]
stl = data_list[7]
blk = data_list[8]
to = data_list[9]
pf = data_list[10]
fga = data_list[11]
fgm = data_list[12]
fta = data_list[13]
ftm = data_list[14]
tpa = data_list[15]
tpm = data_list[16]
efficiency = ((pts+reb+asts+stl+blk)-(fgm-ftm-to))/gp
player_tuple = efficiency, last_name, first_name, team_name
player_list.append(player_tuple)
return sorted(player_list, reverse=True)
def print_results(lst, how_many=50):
"""Print the result in a nice format"""
template = '{:<20}{:<20s}, {:<15s}{:<5s}'
print("The top {} players based on efficiency are: ".format(how_many))
print('*'*75)
for tup in lst[:how_many]:
print(template.format(tup[0], tup[1], tup[2], tup[3]))
if __name__ == "__main__":
file_name1 = input("File name: ")
result_list = get_data_list(file_name1)
print_results(result_list)
Now I'm going to smooth it out even further. This is using more advanced features in Python, but they are features that make things more convenient, not things that are just tricky.
First, instead of building a list with a list comprehension, and then picking items by index number, we will use a generator expression and directly unpack the items into variable names. A generator expression is just like a list comprehension, except that instead of building a list, it provides an "iterator" that can be looped over, or can be unpacked into variable names as I show here.
Second, in the print function, we just want to print all the values in the tuple, in order. Python provides a shortcut: putting a *
in front of the tuple inside the call to .format()
means "unpack this and use the unpacked values as the arguments to this function call".
def get_data_list (file_name):
player_list=[]
with open(file_name, "r") as f:
for line in f:
# Split line on commas and convert each item to integer. Unpack
# values directly into variable names. We are using a
# generator expression to convert all the items to integer,
# and Python's ability to unpack an iterator into a tuple.
items = line.strip().split(',')
# use list slicing to select just the three string values
first_name, last_name, team_name = (s.strip() for s in items[2:5])
# Use a generator expression to convert all values to int.
# Unpack directly to variable names using tuple unpacking.
# Put parentheses so Python won't worry about multiple lines
# of variable names.
(
gp, mins, pts, oreb, dreb, reb, asts,
stl, blk, to, pf, fga, fgm, fta, ftm,
tpa, tpm
) = (int(x) for x in items[6:])
efficiency = ((pts+reb+asts+stl+blk)-(fgm-ftm-to))/gp
player_tuple = efficiency, last_name, first_name, team_name
player_list.append(player_tuple)
return sorted(player_list, reverse=True)
def print_results(lst, how_many=50):
"""Print the result in a nice format"""
template = "{:<20}{:<20s}, {:<15s}{:<5s}"
print("The top {} players based on efficiency are: ".format(how_many))
print('*'*75)
for player_tuple in lst[:how_many]:
print(template.format(*player_tuple))
if __name__ == "__main__":
file_name1 = input("File name: ")
result_list = get_data_list(file_name1)
print_results(result_list)
EDIT: And here is another edited version. This one splits out the logic for parsing a line into a player_tuple
into its own function. This makes get_data_list()
very short.
def player_tuple(line):
# Split line on commas and convert each item to integer. Unpack
# values directly into variable names. We are using a
# generator expression to convert all the items to integer,
# and Python's ability to unpack an iterator into a tuple.
items = line.strip().split(',')
# use list slicing to select just the three string values
first_name, last_name, team_name = (s.strip() for s in items[2:5])
# use a generator expression to convert all values to int
# unpack directly to variable names using tuple unpacking
(
gp, mins, pts, oreb, dreb, reb, asts,
stl, blk, to, pf, fga, fgm, fta, ftm,
tpa, tpm
) = (int(x) for x in items[6:])
efficiency = ((pts+reb+asts+stl+blk)-(fgm-ftm-to))/gp
return efficiency, last_name, first_name, team_name
def get_data_list(file_name):
with open(file_name, "r") as f:
player_list = [player_tuple(line) for line in f]
return sorted(player_list, reverse=True)
def print_results(lst, how_many=50):
"""Print the result in a nice format"""
template = "{:<20}{:<20s}, {:<15s}{:<5s}"
print("The top {} players based on efficiency are: ".format(how_many))
print('*'*75)
for player_tuple in lst[:how_many]:
print(template.format(*player_tuple))
if __name__ == "__main__":
file_name1 = input("File name: ")
result_list = get_data_list(file_name1)
print_results(result_list)
Now that we have player_tuple()
as a function, we could simplify get_data_list()
even further. I won't repeat the whole program, just the simplified get_data_list()
. This is probably the code I would write if I had to solve this problem.
def get_data_list(file_name):
with open(file_name, "r") as f:
return sorted((player_tuple(line) for line in f), reverse=True)
Here we don't even explicitly build the list. We just make a generator expression that provides all the player_tuple
values, and directly pass that to sorted()
. There is no need for this list to be given a name inside get_data_list()
; it can just be built and returned in one line.