Refactor API Access and Parameter Handling for Enhanced Readability and Efficiency

[vnoelifant]

          As discussed on call:

• kwargs mixed feelings, but definitely handy at times - FYI https://pybit.es/articles/the-arbitrary-keyword-arguments-args-and-kwargs-dont-come-for-free-in-python/
• fetch_data_from_api / fetch_api_data_by_query are pass-through, they don't add anything causing some duplication, it would be better to use the tmdb_api_obj methods directly
• tmdb_api_obj = TMDBApi() is at module scope, this could be useful for a shared object, but it might be safer to instantiate it in the view = more locally scoped / better namespaced.

_Originally posted by @bbelderbos in https://github.com/vnoelifant/movie-search-app/pull/77#discussion_r1452634031_

[vnoelifant]

@bbelderbos I am not quite sure what you mean in the third bullet point above. Can we discuss this on our next call further? Thanks.

[vnoelifant]

@bbelderbos I am not quite sure what you mean in the third bullet point above. Can we discuss this on our next call further? Thanks.

@bbelderbos I was working with the understanding that media.py acts as a service layer between the TMDBApi class (in tmdb_api.py) and views.py. There are many calls in media.py that use tmdb_api_obj methods. So if I instantiate in views.py I think that means I will have to change a lot of code in media.py that uses the tmdb_api_obj. How can I manage this if I move the instantiation of TMDBApi to views.py? I thought having the API methods and related object instantiation outside of views.py was a better separation of concerns, but open to your thoughts if I am missing something!

[vnoelifant]

@bbelderbos Curious your take on the recommendation here for handling my concern above. https://chat.openai.com/c/6a7a3432-938a-493f-a769-e1b2587982d8 (dependancy injection, but not too familiar on this).

[bbelderbos]

@bbelderbos Curious your take on the recommendation here for handling my concern above. https://chat.openai.com/c/6a7a3432-938a-493f-a769-e1b2587982d8 (dependancy injection, but not too familiar on this).

Can you share again? I cannot load this conversation ...

[bbelderbos]

@bbelderbos I am not quite sure what you mean in the third bullet point above. Can we discuss this on our next call further? Thanks.

This goes back to the risk of using the global namespace ->

1. Global objects can make unit testing more difficult.

2. Global objects can lead to tight coupling between different parts of an application making code less modular and harder to refactor or extend.

3. In a multi-threaded application, a single, globally accessible object that was not designed for concurrent access can lead to race conditions and data corruption.

4. Global objects add to the module's global namespace increasing the risk of name collisions (more a thing in larger projects).

Instantiating tmdb_api_obj more locally can lead to better-structured, more testable, and flexible code. However, as this is a relatively small project it probably does not matter too much.

[bbelderbos]

@bbelderbos I am not quite sure what you mean in the third bullet point above. Can we discuss this on our next call further? Thanks.

@bbelderbos I was working with the understanding that media.py acts as a service layer between the TMDBApi class (in tmdb_api.py) and views.py. There are many calls in media.py that use tmdb_api_obj methods. So if I instantiate in views.py I think that means I will have to change a lot of code in media.py that uses the tmdb_api_obj. How can I manage this if I move the instantiation of TMDBApi to views.py? I thought having the API methods and related object instantiation outside of views.py was a better separation of concerns, but open to your thoughts if I am missing something!

Let's talk more about this tomorrow looking at the code together ...

[bbelderbos]

Regarding args and kwargs, in the case of movie_search/decorators.py def wrapper(*args, **kwargs): this is totally common and desired, because you want them to be flexible.

In media.py kwargs is mostly used to pass it through to get_data_from_endpoint which then get used in:

            params = {"api_key": self.api_key, "language": self.language}
            params.update(kwargs)
            response = requests.get(url, params=params)

This is probably fine because the API likely discards extra kwargs. However, if it returns errors for unknown parameters, your method could cause unintended errors. This behavior can vary between different APIs.

It's good to be mindful that **kwargs are less explicit, because anything can be passed into the function and Zen says: explicit is better than implicit.

So yes here it's probably fine and overall **kwargs offer flexibility, they can however affect usability, clarity, and error handling.

Therefore it's good practice to handle and document your use of **kwargs carefully to minimize potential issues.

[bbelderbos]

Regarding "tmdb_api_obj = TMDBApi() is at module scope" we were talking about today ...I cannot share the GPT thread because it contained a screenshot so here is additional info related to what I explained on our call today:

and

Makes sense? Let me know if you have any follow up questions about this interesting topic.

[vnoelifant]

Regarding args and kwargs, in the case of movie_search/decorators.py def wrapper(*args, **kwargs): this is totally common and desired, because you want them to be flexible.

In media.py kwargs is mostly used to pass it through to get_data_from_endpoint which then get used in:

            params = {"api_key": self.api_key, "language": self.language}
            params.update(kwargs)
            response = requests.get(url, params=params)

This is probably fine because the API likely discards extra kwargs. However, if it returns errors for unknown parameters, your method could cause unintended errors. This behavior can vary between different APIs.

It's good to be mindful that **kwargs are less explicit, because anything can be passed into the function and Zen says: explicit is better than implicit.

So yes here it's probably fine and overall **kwargs offer flexibility, they can however affect usability, clarity, and error handling.

Therefore it's good practice to handle and document your use of **kwargs carefully to minimize potential issues.

Thank you @bbelderbos. That is helpful. I have a function called get_movie_discover_data and below I am showing the current implementation and previous implementation for handling its parameters. I like the idea of using the kwargs to avoid having so many parameters in get_movie_discover_data, but do see how it is less readable. However, you mentioned documenting kwargs is useful. Are you talking about via docstrings? Let me know how you would handle these many parameters in get_movie_discover_data. For me I am thinking keep with kwargs but add docstrings to help explain the parameters (if that is something Pythonistas do with kwargs). Thanks!

Current implementation:

 def get_movie_discover_data(self, **kwargs):
        # Process genres
        genre_names = kwargs.get("genre_names", [])
        genres = self.get_genres_from_discover(genre_names, MovieGenre)

        # Process person
        person_name = kwargs.get("person_name")
        person_id = None
        if person_name:
            person_service = PersonService()
            person_data = person_service.fetch_person_data(person_name)
            person_id = person_service.get_person_id(person_data, person_name)

        # Process providers
        watch_provider_names = kwargs.get("watch_provider_names", [])
        providers = self.get_providers_from_discover(
            watch_provider_names, MovieProvider
        )

        # Update kwargs with processed data
        kwargs.update(
            {
                "with_genres": ",".join(map(str, genres)),
                "with_watch_providers": ",".join(map(str, providers)),
                "with_people": person_id,
            }
        )

        # Remove None values from kwargs
        kwargs = {k: v for k, v in kwargs.items() if v is not None}

        return self.get_discover_data("/discover/movie", **kwargs)

Previous implementation (can also have default parameters being equal to None)

def get_movie_discover_data(
     genres, person_id, sort_options, region, watch_region, providers, year
):
    return tmdb_api.get_data_from_endpoint(
        "/discover/movie",
        region=region,
        primary_release_year=year,
        with_genres=list(genres),
        sort_by=sort_options,
        watch_region=watch_region,
        with_watch_providers=list(providers),
        with_people=person_id,
    )

[vnoelifant]

@bbelderbos Curious your take on the recommendation here for handling my concern above. https://chat.openai.com/c/6a7a3432-938a-493f-a769-e1b2587982d8 (dependancy injection, but not too familiar on this).

Can you share again? I cannot load this conversation ...

https://chat.openai.com/share/239bb121-48bd-4197-af57-2dc8fbb239f0. Not sure if you can see that update, but in the PR #85 I added a couple of screenshots of the chat. Thanks!

[vnoelifant]

Regarding "tmdb_api_obj = TMDBApi() is at module scope" we were talking about today ...I cannot share the GPT thread because it contained a screenshot so here is additional info related to what I explained on our call today:

and

Makes sense? Let me know if you have any follow up questions about this interesting topic.

@bbelderbos Very interesting, thanks! How can I determine whether this media app needs to be stateless or not? Any questions I should ask myself to determine this? I do know that I'd like flexibility for users to select different query parameters, (i.e. language, year, etc). I think I am leaning toward a middleware to attach query parameters to the request object. https://docs.djangoproject.com/en/5.0/topics/http/middleware/

[vnoelifant]

@bbelderbos I know you mentioned try to remove the pass through methods that were in media.py as per your first comment in this issue, however I am encountering a situation where I have a mixture of calling the methods of the TMDBApi class in tmdb_api.py directly in views.py, and also accessing those methods via the service layers defined in media.py. I am looking for a balance between keeping API interactions in media.py, but also avoiding pass-through methods there as well. Here is some GPT research. Let me know your thoughts!

[bbelderbos]

Thanks for the research, maybe easier to discuss on a call, shall we meet shortly later this week?

[vnoelifant]

Thanks for the research, maybe easier to discuss on a call, shall we meet shortly later this week?

Yeah that sounds good. I can meet anywhere from 2-3 pm (7-8 am my time). @bbelderbos

[bbelderbos]

Great, I sent you an invite for Friday, talk then!