3
\$\begingroup\$

Project collects football data from external API.

from app.teams.get_team_statistics import GetTeamStatistics


class TeamStatisticsService:

    LEAGUE = 'premier league'
    COUNTRY = 'england'

    def __init__(self, statistics_call=None):
        """
        Store a request function used to make API calls.
        """
        self._statistics_call = statistics_call or GetTeamStatistics()
        self._cache = {}

    def _get_response(self, team: str, season: int) -> dict:
        """Fetch and return the API response dict for a given team and season."""
        key = (team, season)
        if key not in self._cache:
            self._cache[key] = self._statistics_call.get_team_statistics_response(
                team=team, league=self.LEAGUE, country=self.COUNTRY, season=season)['response']
        return self._cache[key]

    def team_form(self, team: str, season: int) -> str:
        return self._get_response(team, season)['form']

Testing the API response:

import pytest
from unittest.mock import patch
from app.teams.team_statistics_service import TeamStatisticsService
from app.teams.get_team_statistics import GetTeamStatistics

FAKE_TEAMS = {"response": [{"team": {"id": 33, "name": "Manchester United"}}]}
FAKE_SEASONS = {"response": [2022]}


# Prevents using the real API
@pytest.fixture(autouse=True)
def mock_external_calls():
    with patch('app.teams.get_team_statistics.get_all_teams', return_value=FAKE_TEAMS), \
            patch('app.teams.get_team_statistics.get_season_availability', return_value=FAKE_SEASONS):
        yield


def fake_make_request(request_url: str):
    return {
        "response": [{
            "league": {
                "id": 39,
                "name": "Premier League",
                "country": "England",
                "season": 2022,
            },
            "team": {
                "id": 33,
                "name": "Manchester United",
            },
            "form": "LLWWWWLWDWDWLWWWWWDLWDWWLDLWWWDWLLWWWW",
            "fixtures": {
                "played": {"home": 19, "away": 19, "total": 38},
                "wins": {"home": 15, "away": 8, "total": 23},
                "draws": {"home": 3, "away": 3, "total": 6},
                "loses": {"home": 1, "away": 8, "total": 9},
            },
            "goals": {
                "for": {
                    "total": {"home": 36, "away": 22, "total": 58},
                    "average": {"home": "1.9", "away": "1.2", "total": "1.5"},
                    "minute": {
                        "0-15": {"total": 6, "percentage": "10.53%"},
                        "16-30": {"total": 8, "percentage": "14.04%"},
                        "31-45": {"total": 10, "percentage": "17.54%"},
                        "46-60": {"total": 10, "percentage": "17.54%"},
                        "61-75": {"total": 9, "percentage": "15.79%"},
                        "76-90": {"total": 10, "percentage": "17.54%"},
                        "91-105": {"total": 4, "percentage": "7.02%"},
                        "106-120": {"total": None, "percentage": None},
                    },
                    "under_over": {
                        "0.5": {"over": 31, "under": 7},
                        "1.5": {"over": 20, "under": 18},
                        "2.5": {"over": 6, "under": 32},
                        "3.5": {"over": 1, "under": 37},
                        "4.5": {"over": 0, "under": 38},
                    },
                },
                "against": {
                    "total": {"home": 10, "away": 33, "total": 43},
                    "average": {"home": "0.5", "away": "1.7", "total": "1.1"},
                    "minute": {
                        "0-15": {"total": 6, "percentage": "13.64%"},
                        "16-30": {"total": 6, "percentage": "13.64%"},
                        "31-45": {"total": 7, "percentage": "15.91%"},
                        "46-60": {"total": 7, "percentage": "15.91%"},
                        "61-75": {"total": 7, "percentage": "15.91%"},
                        "76-90": {"total": 9, "percentage": "20.45%"},
                        "91-105": {"total": 2, "percentage": "4.55%"},
                        "106-120": {"total": None, "percentage": None},
                    },
                    "under_over": {
                        "0.5": {"over": 21, "under": 17},
                        "1.5": {"over": 9, "under": 29},
                        "2.5": {"over": 5, "under": 33},
                        "3.5": {"over": 3, "under": 35},
                        "4.5": {"over": 2, "under": 36},
                    },
                },
            },
            "biggest": {
                "streak": {"wins": 5, "draws": 1, "loses": 2},
                "wins": {"home": "4-1", "away": "0-2"},
                "loses": {"home": "1-2", "away": "7-0"},
                "goals": {
                    "for": {"home": 4, "away": 3},
                    "against": {"home": 2, "away": 7},
                },
            },
            "clean_sheet": {"home": 11, "away": 6, "total": 17},
            "failed_to_score": {"home": 2, "away": 5, "total": 7},
            "penalty": {
                "scored": {"total": 3, "percentage": "100.00%"},
                "missed": {"total": 0, "percentage": "0%"},
                "total": 3,
            },
            "cards": {
                "yellow": {
                    "0-15": {"total": 4, "percentage": "5.13%"},
                    "16-30": {"total": 8, "percentage": "10.26%"},
                    "31-45": {"total": 8, "percentage": "10.26%"},
                    "46-60": {"total": 13, "percentage": "16.67%"},
                    "61-75": {"total": 16, "percentage": "20.51%"},
                    "76-90": {"total": 18, "percentage": "23.08%"},
                    "91-105": {"total": 11, "percentage": "14.10%"},
                    "106-120": {"total": None, "percentage": None},
                },
                "red": {
                    "0-15": {"total": None, "percentage": None},
                    "16-30": {"total": None, "percentage": None},
                    "31-45": {"total": 1, "percentage": "50.00%"},
                    "46-60": {"total": None, "percentage": None},
                    "61-75": {"total": 1, "percentage": "50.00%"},
                    "76-90": {"total": None, "percentage": None},
                    "91-105": {"total": None, "percentage": None},
                    "106-120": {"total": None, "percentage": None},
                },
            },
        }]
    }


TEAM = 'manchester united'
SEASON = 2022


class TestTeamStatisticsService:

    def setup_method(self):
        mock_get_team_stats = GetTeamStatistics(request_fn=fake_make_request)
        self.service = TeamStatisticsService(
            statistics_call=mock_get_team_stats)

    # ------------------------------------------------------------------ #
    # Form                                                                 #
    # ------------------------------------------------------------------ #

    def test_team_form(self):
        assert self.service.team_form(
            TEAM, SEASON) == "LLWWWWLWDWDWLWWWWWDLWDWWLDLWWWDWLLWWWW"

Questions/comments: I'm not sure whether self._statistics_call = statistics_call or GetTeamStatistics() in the test setup is the cleanest way? I aimed to not use real API calls in tests. Are the fixtures the best way to setup the test with the mock data? The cacheing prevents repeated calls if i already have the data, is there a clearer/better way to do this?

\$\endgroup\$

2 Answers 2

Reset to default
5
\$\begingroup\$

Kudos on this nice, idiomatic disjunction:

    self._statistics_call = statistics_call or GetTeamStatistics()

naming a callable

        self._statistics_call = ...

In that spot it appears you wanted a "function" noun, rather than a "please call() me" verb:

        self._statistics_fn = ...

This parameter seems like a good name:

        ... = GetTeamStatistics(request_fn=fake_make_request)

vague return type

    def _get_response(self, team: str, season: int) -> dict:

Consider telling maintainers (and linters) that we return a mapping from tuple to string: -> dict[tuple[str, int], str]:

Thank you for this lovely signature; it is very informative.

    def team_form(self, team: str, season: int) -> str:

verbose identifiers

... import TeamStatisticsService
... import GetTeamStatistics

These are very nice identifiers. But we could easily forgive you if you chose to shorten them a little:

  • TeamStatsSvc
  • GetTeamStats

Also, the verb "get" seems slightly odd in a class name, where we expect to find a noun. Maybe "getter"? Or "team query service"?

fake out

FAKE_TEAMS = {"response": [{"team": {"id": 33, "name": "Manchester United"}}]}
FAKE_SEASONS = {"response": [2022]}

I guess "fake" isn't the worst prefix for such variables. But maybe "mock", or "test"? Or just elide the prefix?

def fake_make_request( ... ):

That feels more like a mock_request() to me. Or more descriptively, a mock_request_results() or ..._response().

memoizing

        if key not in self._cache:

caching prevents repeated calls ... , is there a clearer / better way to do this?

Using the @cache decorator (or the LRU variant) is the usual idiomatic way to accomplish your aim. It would be instantly recognized by any maintainer who engages with your codebase.

\$\endgroup\$
2
  • \$\begingroup\$ Great advice! Would -> dict[tuple[str, int], str]: be better as -> dict[str, dict]: ? . Output = {'league': {'id': 39, 'name': 'Premier League', 'country': 'England', 'season': 2022}, 'team': ... \$\endgroup\$ Commented yesterday
  • \$\begingroup\$ Ummm, maybe I misread what your key is? I kind of thought the integer season was in there, but I'm certainly prepared to say I was wrong. If we're mapping from string to a dictionary, then yeah, annotate it as such. Of course mapping to just a dict is on the vague side, and mypy --strict or pyright might call you out on it. Perhaps we're mapping to a dict[str, int|str], or mapping to a dict[str, Any] ? \$\endgroup\$ Commented yesterday
5
\$\begingroup\$

Not a direct answer to your question, but commentary:

JSON

This long function feels like it should really be loading a JSON file rather than hard-coding in the test data.

def fake_make_request(request_url: str):
    ...
\$\endgroup\$

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.