4
\$\begingroup\$

I am writing a program which calculates the scores for participants of a small "Football Score Prediction" game.

Rules are:

  1. if the match result(win/loss/draw) is predicted correctly: 1 point
  2. if the match score is predicted correctly(exact score): 3 points (and +1 point because 1st rule is automatically satisfied)

Score is calculated after every round of matches (10 matches in a round).

The program I've written is as follows:

import numpy as np import re players = {0:'Alex',1:'Charlton',2:'Vineet'} results = np.array(range(40),dtype='a20').reshape(4,10) eachPrediction = np.array(range(20),dtype='a2').reshape(2,10) score = np.zeros(3) correctScore=3 correctResult=1 allPredictions = [] done = False def takeFixtures(): filename='bplinput.txt' file = open(filename) text = file.readlines() fixtures = [line.strip() for line in text] file.close() i=0 for eachMatch in fixtures: x=re.match("(.*) ([0-9]+) ?- ?([0-9]+) (.*)", eachMatch) results[0,i]=x.group(1) results[1,i]=x.group(2) results[2,i]=x.group(3) results[3,i]=x.group(4) i+=1 def takePredictions(noOfParticipants): for i in range(0,noOfParticipants): print("Enter predictions by "+players[i]+" in x-y format") for i in range(0,10): eachFixturePrediction = raw_input("Enter prediction for "+results[0,i]+" vs "+results[3,i]+": ") x=eachFixturePrediction.split('-') eachPrediction[0,i]=str(x[0]) eachPrediction[1,i]=str(x[1]) allPredictions.append(eachPrediction) def scoreEngine(): for i in range(0,len(players)): for j in range(0,10): resultH=int(results[1,j]) resultA=int(results[2,j]) result=resultH-resultA predictionH=int(allPredictions[i][0][j]) predictionA=int(allPredictions[i][1][j]) pResult = predictionH-predictionA if result == pResult or (result<0 and pResult<0) or (result>0 and pResult>0): score[i]+=correctResult if resultH==predictionH and resultA==predictionA: score[i]+=correctScore noOfParticipants=len(players) takeFixtures() takePredictions(noOfParticipants) scoreEngine() print("Scores are:") print(score) for player in players: print(players[player]+" has scored "+ str(score[player])+" points" )

The file used to take list of fixtures looks like this:

West Bromwich Albion 0-1 Tottenham Hotspur Manchester City 2-2 Liverpool Queens Park Rangers 0-0 Norwich City Arsenal 1-0 Stoke City Everton 3-3 Aston Villa Newcastle United 3-2 Chelsea Reading 2-1 Sunderland West Ham United 1-0 Swansea City Wigan Athletic 2-2 Southampton Fulham 0-1 Manchester United

I want advice on how this program can be improved in any way (decreasing the code/ making it efficient).

\$\endgroup\$

3 Answers 3

Reset to default
6
\$\begingroup\$

Style

The first thing to do is to follow PEP 8: it's a style guide that says how you should indent your code, name your variables, and so on. For example, prefer results[0, i] = x.group(1) to results[0,i]=x.group(1).

Files

Opening files in Python should be done using the with idiom: your file is then guaranteed to be close correctly. takeFixtures now becomes:

def takeFixtures(): with open('bplinput.txt') as file: for eachMath in file: x=re.match("(.*) ([0-9]+) ?- ?([0-9]+) (.*)", eachMatch.strip) results[0,i]=x.group(1) results[1,i]=x.group(2) results[2,i]=x.group(3) results[3,i]=x.group(4) i+=1

Data structures

I you didn't use numpy, you could have written results[i] = x.groups(), and then used zip(*results) to transpose the resulting matrix. More generally, as pointed out by Josay, you should use Python data structures, and only switch to numpy if you find out that this is where the inefficiency lies.

Formatting

The last thing that the other reviewers didn't mention is that concatenating strings is poor style, and format() should be preferred since it's more readable and more efficient:

for player in players: print("{} has scored {} points".format(players[player], score[player]))
\$\endgroup\$
2
  • \$\begingroup\$ thanks,Can you give me suggestion on a better way to take input from all the players? right now I have to manually input the score predictions by all players, and if there is some mistake i have to do it all over again. ie. one mistake and i have to input 30 predictions all over again \$\endgroup\$ Commented Feb 7, 2013 at 15:29
  • 1
    \$\begingroup\$ You need to catch the exception that comes out and ask the input to be made again. You would need some kind of loop to make the input until it worked correctly. This would be a good question on StackOverflow unless it has alread been asked. \$\endgroup\$ Commented Feb 7, 2013 at 16:14
5
\$\begingroup\$

First thing, let's talk about the players array :

  • you define it with players = {0:'Alex',1:'Charlton',2:'Vineet'}.

  • you iterate on it using :

    for i in range(0,noOfParticipants): # Something about : players[i]

    or

    for player in players: # Something about : players[player] and str(score[player])

First passing the length of an array without passing the array is a bit awkward (not to say useless). Then, the way you loop is not very pythonic. If what you want is a list of names, define it as a list of names: players = ['Alex','Charlton','Vineet'] (if you define the index yourself and you iterate using only the size of the loop, you might get troubles). Then, if you want to iterate over the names, it's pretty straightforward: for player in players:. You want the index as well ? Enumerate is what you want: for i,name in enumerate(players). How cool is this ?

However, it might be a good idea to make this array a little bit more complicated and use it to store not only the players' names but also their prediction and their score. Then, a dictionary would be a good solution.

Then, a few details :

  • I think you should get rid of the magic numbers: I have no idea what the different numbers are for.

  • I think correctScore and correctResult are not very good names to express number of points (pointCorrectScore and pointCorrectResult are my suggestions) but storing this in variable is a pretty good idea. Also, I would advise you to create a function returning the score from an estimation and an actual score. Your variables correctScore and correctResult could then be local to your function.

I have no time to go further in the code I didn't quite understand at first glance but I'll try to do it eventually.

\$\endgroup\$
4
\$\begingroup\$

I'd change something in the style of your application in order to decrease the coupling.

Why do you use global variables? I'd change the design to have the following functions:

  • takeFixtures(fileName) returning the fixtures;
  • takePredictions(numberOfParticipants, fixtures) returning the predictions for each player. This should be decomposed in a function that takes the predictions of a player and another that takes the prediction for a single game;
  • computeScores(fixtures, predictions) that returns a list of pair (player, score). That function should be decomposed to other functions computing the score for a player and the score for a single game.

In addition to that, why do you limit your code to work only if you have 10 games? Wouldn't it be better if you replace the 10 in for j in range(0,10): with len(games) or something similar? That would also benefit from the functional point of view as your code would support out of the box any league regardless of the number of games played each turn.

\$\endgroup\$
1
  • \$\begingroup\$ the reason I limited to 10 games because its for Barclays premier league which has 20 teams. but its a good suggestion, i'll take it :) thanks \$\endgroup\$ Commented Feb 7, 2013 at 15:23

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.