4
\$\begingroup\$

I have created the following code:

class Club(object):
    def __init__(self, name, points=1000):
        self.name = name
        self.points = points

    def __eq__(self, other):
        return self.name == other.name

    def __gt__(self, other):
        return self.points > other.points

    def compute_points(self, opponent, is_home, result):
        rating_diff = self.points - opponent.points + (100 if is_home else -100)
        expected = 1 / (1 + 10**(-rating_diff/400))
        return 32 * (result-expected)


def play_match(home, away, result):
    home_change = int(home.compute_points(away, True, result))
    away_change = int(away.compute_points(home, False, 1-result))
    home.points += home_change
    away.points += away_change

def club_index(club_to_find, clubs):
    index = 0
    while index < len(clubs):
        club = clubs[index]
        if club.name == club_to_find:
            return index
        index += 1
    return -1

def main():
    clubs = []
    with open("matches.txt") as file:
        matches = file.readlines()

    for match in matches:
        (home_str, away_str, result) = match.split(" ")

        index = club_index(home_str, clubs)
        if index == -1:
            home = Club(home_str)
            clubs.append(home)
        else:
            home = clubs[index]

        index = club_index(away_str, clubs)
        if index == -1:
            away = Club(away_str)
            clubs.append(away)
        else:
            away = clubs[index]

        play_match(home, away, float(result))

    clubs = sorted(clubs)
    clubs.reverse()
    with open("ranking.txt", "w") as file:
        for club in clubs:
            file.write(club.name + ": " + str(club.points) + "\n")

if __name__ == "__main__":
    main()

which takes a file containing match results, and creates a file containing a ranking based on the Elo Ranking System. However, I feel like my main() method could've been implemented in a better way, so I came here to see for some pointers as to how I can better implement it or pick up any other good practices.

EDIT: Here's an example of the input file:

Swansea ManUtd 0
WestHam Cardiff 1
WestBrom Southampton 0
Sunderland Fulham 0
Norwich Everton 0.5
Arsenal AstonVilla 0
Liverpool Stoke 1
Chelsea Hull 1
\$\endgroup\$
1
  • \$\begingroup\$ Watch out when implementing only some of __eq__, __ne__, __lt__, __gt__, etc., or __eq__ and __hash__. It can make a class harder to use correctly. \$\endgroup\$ Dec 8, 2013 at 21:56

1 Answer 1

6
\$\begingroup\$

In play_match, the Club class should be responsible for updating its own score. You could define an update_points method taking the same parameter as the computes_point one (and calling it). Also, shouldn't compute_points be responsible for the conversion to int ?

I find it a bit awkward to have the __eq__ and the __gt__ base their logic on completely different fields.

In a club_index : as a general rule, you do not iterate that way over object in Python. The pythonic way to write

def club_index(club_to_find, clubs):
    index = 0
    while index < len(clubs):
        club = clubs[index]
        if club.name == club_to_find:
            return i
        index += 1
    return -1

is

def club_index(club_to_find, clubs):
    for i,club in enumerate(clubs)
        if club.name == club_to_find:
            return index
    return -1

Now, if I was to write such a program, I wouldn't even create a class ( you can have a look at http://www.youtube.com/watch?v=o9pEzgHorH0 ). A dictionnary to map club names to score is pretty much everything you need but that's a different way to see the problem I guess.

Edit : Actual tested code added.

def get_new_scores(home_score,away_score,result):
    rating_diff = home_score - away_score + 100
    home_expected = 1 / (1 + 10**(-rating_diff/400))
    away_expected = 1 / (1 + 10**(+rating_diff/400))
    home_return = int(32 * ((result  )-home_expected))
    away_return = int(32 * ((1-result)-away_expected))
    return home_score+home_return,away_score+away_return

def main():
    with open("club.txt") as file:
        matches = file.readlines()

    clubs = dict()
    for match in matches:
        (home_str, away_str, result) = match.split(" ")
        clubs[home_str],clubs[away_str] = get_new_scores(clubs.setdefault(home_str,1000), clubs.setdefault(away_str,1000), float(result))

    for club in sorted(clubs, key=clubs.get, reverse=True):
        print(club + ": " + str(clubs[club]) + "\n")

if __name__ == "__main__":
    main()
\$\endgroup\$
3
  • \$\begingroup\$ Could you please provide an explanation or link as to what setdefault is because I've never seen it before and can't quite make sense of it by Googling it or trying to find it in the docs. \$\endgroup\$ Dec 8, 2013 at 21:57
  • 1
    \$\begingroup\$ @LazySloth13 See docs.python.org/2/library/stdtypes.html#dict.setdefault \$\endgroup\$ Dec 9, 2013 at 10:19
  • \$\begingroup\$ The pythonic loop should return its index variable i rather than the undefined index on finding a match. And in your tested code, the first line of main() seems useless, since clubs = dict() will discard the existing value, which has never been used. \$\endgroup\$ Dec 9, 2013 at 17:05

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.