Retrieve player rating based on defined criteria

Question

Using the chess.com API, I take in a list of players and return their ratings based on a certain criteria -for instance, they have to be a member of a certain club, they have to had played a number of certain games, and they should have played a certain number of games within a certain number of months among other things.

If a player meets all the criteria, their username, rating, and the rating type is returned in a 3-tuple. Two other possible returns are also provided, 1 is a different rating type which is also a 3-tuple or if they had closed their accounts -for example- it returns a 2-tuple -containing their username and a string mentioning they closed their accounts.

I have 5 classes and they are the following:

Warnings: Contains methods that just issues certain warnings that will be used later on.

Player: Mainly methods that are general requests to endpoints for a player. Such as getting the account status of the player or getting their stats.

GamePlay: Contains methods that focus on defining way so of getting a players games and general "game criteria".

PlayerCriteria: As the name suggests, the methods define the criteria for a player that will later be used when fetching a player's rating.

RatingFetcher: Focuses on fetching the rating.

#! python
# -*- coding: utf-8 -*-
""""Retrieves player rating based on defined criteria."""
from datetime import date
from dateutil.relativedelta import relativedelta
from warnings import warn
from operator import itemgetter

from requests import get


__author__ = "Walid Mujahid وليد مجاهد"
__copyright__ = "Copyright 2017, Walid Mujahid وليد مجاهد"
__credits__ = ["Walid Mujahid وليد مجاهد"]
__license__ = "MIT"
__version__ = "0.7.0"
__maintainer__ = "Walid Mujahid وليد مجاهد"
__email__ = "[email protected]"
__status__ = "Development"


REQUEST_HEADERS = {'User-Agent': 'RatingsFetcher/0.7.0 '
                                 '(Author: Walid Mujahid, '
                                 'Email: [email protected], '
                                 'Chess.com username: walidmujahid)'}


class Warnings:
    """TODO: Docstring"""
    def __init__(self, username):
        """TODO: Docstring"""
        self.username = username

    def has_not_played_minimum_standard_games(self):
        """TODO: Docstring"""
        warn(f"{self.username} has not played minimum amount"
             " of standard games. Blitz rating may be used.")

    def has_not_played_minimum_blitz_games(self):
        """TODO: Docstring"""
        warn(f"{self.username} has not played minimum amount"
             " of blitz games.")

    def has_closed_account(self):
        """TODO: Docstring"""
        warn(f"{self.username} has closed their account.")

    def has_violated_fair_play_rules(self):
        """TODO: Docstring"""
        warn(f"{self.username} has violated the fair play rules.")

    def is_not_a_member_of_the_nspcl(self):
        """TODO: Docstring"""
        warn(f"{self.username} is not a member of the Not-So "
             f"PRO Chess League.")

    def is_a_titled_player(self):
        """TODO: Docstring"""
        warn(f"{self.username} is a titled player.")


class Player:
    """TODO: Docstring"""
    def __init__(self, username):
        """TODO: Docstring"""
        self.username = username

    def get_account_status(self):
        """TODO: Docstring"""
        return get(f"https://api.chess.com/pub/player/{self.username}"
                   ).json()['status']

    def get_player_stats(self):
        """TODO: Docstring"""
        return get(f"https://api.chess.com/pub/player/{self.username}/stats",
                   headers=REQUEST_HEADERS).json()


class GamePlay:
    """TODO: Docstring."""
    def __init__(self, username: str, past_months: int=8):
        """TODO: Docstring"""
        self.username = username
        self.past_months = past_months

    def has_played_x_number_of_games_of_type(
            self, game_type: str, minimum_number_of_games_played: int):
        """TODO: Docstring"""
        game_count = self.count_live_chess_games_of_type(game_type)

        return True if game_count >= minimum_number_of_games_played else False

    @staticmethod
    def is_game_of_type(game: dict, game_type: str):
        """Checks if the game is of a certain type - like 'standard' or 'blitz'.
        """
        if game['time_class'] == game_type:
            return True
        else:
            return False

    def generate_month_range(self):
        """TODO: Docstring"""
        today = date.today()
        list_of_months_in_range = []
        custom_number_of_months_ago = today + relativedelta(
            months=-self.past_months)

        while custom_number_of_months_ago <= today:
            list_of_months_in_range.append(custom_number_of_months_ago)
            custom_number_of_months_ago += relativedelta(months=1)

        return list_of_months_in_range

    @staticmethod
    def request_monthly_archive(url: str):
        """Gets monthly archive using url"""
        return get(url, headers=REQUEST_HEADERS).json()['games']

    def get_monthly_archives(self, year: int, month: int):
        """Get a monthly archive for custom year and month."""
        url = f'https://api.chess.com/pub/player/' \
              f'{self.username}/games/{year}/{month:02d}'

        return self.request_monthly_archive(url)

    def get_live_chess_games_of_type(self, game_type: str):
        """TODO: Docstring"""
        games_of_type = []

        for dates in self.generate_month_range()[::-1]:
            games = self.get_monthly_archives(dates.year, dates.month)

            for game in games:
                if self.is_game_of_type(game, game_type):
                    games_of_type.append(game)

        return sorted(games_of_type, key=itemgetter('end_time'))

    def count_live_chess_games_of_type(self, game_type: str):
        """TODO: Docstring"""
        return len(self.get_live_chess_games_of_type(game_type))


class PlayerCriteria:
    """TODO: Docstring"""
    def __init__(self, username: str):
        """TODO: Docstring"""
        self.username = username
        self.status = Player(username).get_account_status()
        self.player_game = GamePlay(username)

    def is_member_of_nspcl(self):
        """Returns True if player is a member of the NSPCL."""
        nspcl_members = 'https://api.chess.com/pub/club/' \
                        'not-so-pro-chess-league/members'

        response = get(nspcl_members, headers=REQUEST_HEADERS).json()
        members = list(set(response['weekly'] + response['monthly'] +
                           response['all_time']))

        if self.username.lower() in members:
            return True
        else:
            return False

    def has_played_minimum_standard_games(self, minimum_number=10):
        """TODO: Docstring"""
        return self.player_game.has_played_x_number_of_games_of_type(
            'standard', minimum_number)

    def has_played_minimum_blitz_games(self, minimum_number=10):
        """TODO: Docstring"""
        return self.player_game.has_played_x_number_of_games_of_type(
            'blitz', minimum_number)

    # make sure player has not closed their account
    def has_not_closed_account(self):
        """TODO: Docstring"""
        return True if self.status != 'closed' else False

    # make sure player has not violated any fair policy rules
    # and had their account closed
    def has_not_violated_fair_play_rules(self):
        """TODO: Docstring"""
        return True if self.status != "closed:fair_play_violations" else False


class RatingFetcher:
    """TODO: Docstring"""
    def __init__(self, username):
        """TODO: Docstring"""
        self.username = username
        self.warnings = Warnings(username)
        self.player_criteria = PlayerCriteria(username)
        self.player_stats = Player(username).get_player_stats()

        # get rapid and blitz ratings and put them in a tuple that mentions
        # the game type - e.g blitz or rapid
        self.rapid_rating = (username,
                             self.player_stats['chess_rapid']['last']['rating'],
                             'rapid')
        self.blitz_rating = (username,
                             self.player_stats['chess_rapid']['last']['rating'],
                             'blitz')

    def fetch_rating(self):
        """TODO: Docstring"""
        if self.player_criteria.has_not_violated_fair_play_rules():
            if self.player_criteria.has_not_closed_account():
                if self.player_criteria.is_member_of_nspcl():
                    if self.player_criteria.has_played_minimum_standard_games():
                        return self.rapid_rating
                    else:
                        self.warnings.has_not_played_minimum_standard_games()

                    if self.player_criteria.has_played_minimum_blitz_games():
                        return self.blitz_rating
                    else:
                        self.warnings.has_not_played_minimum_blitz_games()
                        return tuple([self.username, 'does not meet criteria'])
                else:
                    self.warnings.is_not_a_member_of_the_nspcl()
                    return tuple([self.username, 'not a member of NSPCL'])
            else:
                self.warnings.has_closed_account()
                return tuple([self.username, 'account closed'])
        else:
            self.warnings.has_violated_fair_play_rules()
            return tuple([self.username, 'violated fair play rules'])


if __name__ == '__main__':
    list_of_players = ['spaceface23', 'walidmujahid', 'ijgeoffrey',
                       'VicMcCracken', 'eoguel', 'tombulous', 'regicidalmaniac']

    for player in list_of_players:
        print(RatingFetcher(player).fetch_rating())

I do have plans on just making a package and separating the classes into individual files. As well as putting the authorship information in an __init__.py.

Currently, the above code works, however, I feel it could be better. I am open to anything that can help me improve it.

Use Cases:

When writing this script, I had wanted it to be generic -general? not sure the right term to use here- enough for multiple use cases. The idea for this "script" was to use it -and parts of it- in more specific tools.

The main use case I had in mind when writing this script was getting the rating for a list of users. Currently the data could be fed in via CSV or maybe even getting it straight from a Google sheet, and then the rating and other information would be given back in either formats -CSV or just writing directly to a Google Sheet. This script would not be handling all of that, but rather it would be used in a tool that would.

Now, there are plans to go beyond a simple CSV / Google Sheet and automate player signups. The idea for this script in that case would be to automatically get a player's rating at sign up according to the criteria -in this case, it would make some of the criteria checks, like the checks for the closed account status unnecessary, however, the idea was to fit other cases like the first mentioned as well as this.

Basically, I was trying to aim for abstract and flexible.

Answer 1

Develop a use case

Looking at your classes, I don't understand what you intend to use them for. The fact that your docstrings are all """TODO: Docstring""" doesn't help with this. So I have to ask you, what problem does this code solve?

You have internalized the HTTP request. So I can't pass in a connection, or a session. This means I have to trust your interface and cannot provide my own certs. Not a great web library.

Your RatingFetcher does a lot of work at init time, then when I call fetch_rating it returns an object that I have to decode based on its length. That's not very intuitive or friendly.

Before you do anything else, I'd suggest you have a clear vision in mind of what problem you're solving, so you can write code that makes that solution Pythonic. I don't see any generators or iterators, so it doesn't seem to handle bulk operations. Is that a drawback or a feature?

What is a Warnings?

Your warnings class takes a username as a parameter. But really, all you do is generate formatted warning messages. And the method names on the class are about the same length as the messages, so if I use your Warnings class I don't think I'm saving anything - no keystrokes, no logic, no convenience.

This goes back to the lack of a use case, but I don't understand why I would ever want to use your Warnings. The kind of information being presented is not typically what the warnings module is used for. You are printing warnings based on data, rather than based on danger to the code. That seems like more of a GUI thing or WebUI thing than a console thing. Don't tell the network admin that a chess player hasn't played enough games - you should be telling the user that is challenging them.

Don't nest too deep

What does else: mean here?

    else:
        self.warnings.has_violated_fair_play_rules()
        return tuple([self.username, 'violated fair play rules'])

You've structured your code in this incredibly indented format, for no good reason that I can see. In fact, you are testing a series of conditions, and when those conditions are met, you are returning. Just structure your code that way - it's easier to understand:

if not self.player_criteria.has_not_violated_fair_play_rules():
    self.warnings.has_violated_fair_play_rules()
    return tuple([self.username, 'violated fair play rules'])

In fact, you might want to invert your criteria methods to remove the not from the method names:

if self.player_criteria.has_violated_fair_play_rules():
    ... etc ...

Encapsulate

Your RatingFetcher has a self.player_criteria object. But in your init:

    self.player_criteria = PlayerCriteria(username)
    self.player_stats = Player(username).get_player_stats()

You create a Player and then throw it away after getting the stats. Why?

Why do you keep the player_criteria but discard the Player? Aren't the criteria (and the stats), in fact, attributes of the Player? I suggest that you merge those objects into the Player interface, and let it decide what or who to dispatch to. (This goes back to me not understanding what a RatingFetcher is, and not understanding why you need one.)

Laziness is a virtue

In your RatingFetcher class __init__ method you make some web calls. This is a bad idea, simply because creating an object doesn't guarantee using the object.

This is demonstrated later in your fetch_rating method. The __init__ method sets self.rapid_rating by calling:

self.player_stats = Player(username).get_player_stats()

This is a web fetch. But you check the criteria:

if self.player_criteria.has_played_minimum_standard_games():
    return self.rapid_rating
else:
    self.warnings.has_not_played_minimum_standard_games()

So if the user has a bad criterion, you return a warning, not the player stats. Meaning the web fetch was wasted traffic!

It would be better to use an @property method, or use a lazy load test, to defer fetching the data until such time that you know you are going to use it.

Return a consistent value

When I call fetch_rating the return value might be a triple. Or it might be a 1-tuple containing a list. Wait, what?

You need to rethink this approach. If I am not allowed to look up a players stats when that player has violated whatever criteria, then there needs to be a consistent return value. Don't make me compute len(result) to determine what you returned from your interface! Either raise an exception (exceptions are cheap in Python, remember) or return a (error, result) tuple always, or something. Make it consistent and easy!

Answer 2

Please remove the TODO lines until you're actually ready to add them. Right now it's just noise and makes the code harder to read.

You've got a repeated anti-pattern here.

def has_played_x_number_of_games_of_type(
          self, game_type: str, minimum_number_of_games_played: int):
      game_count = self.count_live_chess_games_of_type(game_type)

      return True if game_count >= minimum_number_of_games_played else False

A comparison operator already returns a boolean (True or False) so you don't need the if ... else here. Just return game_count >= minimum_number_of_games_played. And similarly below:

def is_game_of_type(game: dict, game_type: str):
    """Checks if the game is of a certain type - like 'standard' or 'blitz'.
    """
    if game['time_class'] == game_type:
        return True
    else:
        return False

I don't know why you did a ternary in the previous function and a multi-line if-else here. But don't bother with either, just return game['time_class'] == game_type

Here's another anti-pattern:

def generate_month_range(self):
    today = date.today()
    list_of_months_in_range = []
    custom_number_of_months_ago = today + relativedelta(
        months=-self.past_months)

    while custom_number_of_months_ago <= today:
        list_of_months_in_range.append(custom_number_of_months_ago)
        custom_number_of_months_ago += relativedelta(months=1)

    return list_of_months_in_range

When writing Python, if you ever initialize a list to [] and then append items to it using a while or for, you're doing something wrong. Sometimes the proper way will be to use a generator:

def generate_month_range(self):
    today = date.today()
    custom_number_of_months_ago = today + relativedelta(
        months=-self.past_months)

    while custom_number_of_months_ago <= today:
        yield custom_number_of_months_ago
        custom_number_of_months_ago += relativedelta(months=1)

    yield break

although you could probably do better. (especially note that rather than using the [::-1] you could generate them in the proper order initially and not need to slice the list)

Same anti-pattern for the below. You really need to read up on list comprehensions.

def get_live_chess_games_of_type(self, game_type: str):
    games_of_type = []

    for dates in self.generate_month_range()[::-1]:
        games = self.get_monthly_archives(dates.year, dates.month)

        for game in games:
            if self.is_game_of_type(game, game_type):
                games_of_type.append(game)

This function should be one list comprehension, as follows:

def get_live_chess_games_of_type(self, game_type: str):
    return [game
            for games in self.get_monthly_archives(dates.year, dates.month)
            for dates in self.generate_month.range()[::-1]
            if self.is_game_of_type(game, game_type)]