Python: Use of class methods to change data

Question

Could you please give me some feedback on the use of class methods in this Python script? The code retrieves time entries from the Toggl API and works as expected. My question is, do the methods make sense? The methods first_api_call and make_subsequent_calls change self.data. Is this okay or should all the changes to self.data be made in retrieve_data?

from datetime import datetime

import requests

today = datetime.today().date()
since = datetime(today.year, 1, 1)


class Toggl:
    def __init__(self):
        self.data = []
        self.total_calls = None
        self.params = {
            'workspace_id': '5604761',
            'since': since,
            'until': today,
            'user_agent': '[email protected]',
            'page': 1,
        }

    def retrieve_data(self):
        r = self.first_api_call()
        self.total_calls = self.calculate_number_of_extra_calls_needed(r)
        self.make_subsequent_calls()

    def first_api_call(self):
        r = requests.get('https://toggl.com/reports/api/v2/details',
                         params=self.params,
                         auth=('APIKEY', 'api_token'))
        r.raise_for_status()
        r = r.json()
        self.data = r['data']
        return r

    def calculate_number_of_extra_calls_needed(self, r):
        """
        Calculate the number of pages that need to be retrieved.
        The second part is True/False to round up.
        Finally minus 1 because we got the first page already.
        """
        total_calls = r['total_count'] // 50 + (r['total_count'] % 50 > 0) - 1
        return total_calls

    def make_subsequent_calls(self):
        """Make the new API calls and store the data"""
        for i in range(self.total_calls):
            self.params['page'] = self.params['page'] + 1
            r = requests.get('https://toggl.com/reports/api/v2/details',
                             params=self.params,
                             auth=('APIKEY', 'api_token'))
            r.raise_for_status()
            r = r.json()
            for d in r['data']:
                self.data.append(d)


if __name__ == "__main__":
    toggl = Toggl()
    toggl.retrieve_data()
    print(toggl.data)

In general is a good practice to have the requests.get operations on a try block, so if there is an issue on the network, or auth your program can continue — camp0
– camp0, Commented Nov 10, 2020 at 11:20

htl · Accepted Answer · 2020-11-10 10:52:50Z

It might be best to move

today = datetime.today().date()
since = datetime(today.year, 1, 1)

into the class's __init__. As it is, the code will just be run once when the module loads, whereas it would make more sense to work out the date when you construct Toggl.

I'd also say that having two methods for what is essentially the exact same task could be streamlined. Perhaps something like this?

import math

# Default page size in the Toggl API.
PAGE_SIZE = 50

class Toggl:
    def __init__(self):
        today = datetime.today().date()
        since = datetime(today.year, 1, 1)
        self.data = []
        self.total_calls = None
        self.params = {
            'workspace_id': '5604761',
            'since': since,
            'until': today,
            'user_agent': '[email protected]'
        }

    def fetch_page(self, page_number):
        params = self.params
        params['page'] = page_number
        r = requests.get('https://toggl.com/reports/api/v2/details',
                         params=params,
                         auth=('APIKEY', 'api_token'))
        r.raise_for_status()
        return r.json()

    def retrieve_data(self):
        r = self.fetch_page(1)
        self.data = r['data']
        pages = self.total_pages(r['total_count'])
        for i in range(2, pages + 1):
            r = self.fetch_page(i)
            self.data += r['data']

    def total_pages(self, total_count):
        return math.ceil(total_count / PAGE_SIZE)

In particular, any "magic number" constants should be taken out and put at the top as constants (by convention in SCREAMING_SNAKE_CASE). I tweaked the function names to be more descriptive, and replaced first_api_call and make_subsequent_calls with a function that doesn't affect the state: instead it just makes a call to fetch the required page and retreive_data decides what to do with it. I don't think there's anything inherently wrong with doing it as you did though, I just personally thought fetch_page seemed cleaner and reduced duplication.

You can simplify the calculation to work out how many pages you need using math.ceil as I demonstrate above too: for example 101 / 50 = 2.02, which is mapped to 3 by math.ceil.

To make the code even better, you could avoid storing the API key hard coded: take it as a parameter in the constructor and store it in self. You could also take since and today as parameters so the user can choose the range that they want to obtain.

Thank you very much, this is exactly what I meant. API responses are almost always paginated. I like how you encapsulated it in the retrieval method. Great other recommendations too, I'll incorporate them all in my code. (BTW: the API keys live in environment variables, just hardcoded them to simplify the example). Thanks again, it is very much appreciated! — PythonSherpa
– PythonSherpa, Commented Nov 10, 2020 at 11:13

Stack Exchange Network

Python: Use of class methods to change data

1 Answer 1

You must log in to answer this question.

Hot Network Questions

Python: Use of class methods to change data

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions