Clean up code #1244
Clean up code #1244
Conversation
Summary of ChangesHello @Ziedelth, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request undertakes a significant code cleanup and refactoring effort across various platform integrations. The core objective is to enhance maintainability and reduce redundancy by centralizing common logic, particularly concerning the determination of when to fetch simulcast episodes and how to convert them. Additionally, it introduces helpful utility methods for navigating between episodes on different platforms, which will streamline future development and improve overall code clarity. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request does a good job of cleaning up the code, especially by centralizing the logic for determining if a simulcast can be fetched (canBeFetch method) and by simplifying the Netflix platform configuration. The refactoring of NetflixPlatform.convertEpisode is also a good improvement.
I've found a critical issue in NetflixWrapper.kt that could lead to a NullPointerException. I've also left a few suggestions for further cleanup by using the new helper methods for finding previous/next episodes, which are currently unused. Applying these suggestions would further improve code clarity and maintainability by reducing duplication.
| requireNotNull(metadata?.thumbnail), | ||
| metadata.banner ?: showJson.getAsJsonObject("boxartHighRes")!!.getAsString("url")!!.substringBefore("?"), | ||
| metadata.carousel ?: showJson.getAsJsonObject("storyArt")!!.getAsString("url")!!.substringBefore("?"), |
There was a problem hiding this comment.
There's a potential NullPointerException here. The metadata object can be null if getMetadata() fails, as it's wrapped in runCatching { ... }.getOrNull(). The current code accesses metadata.banner and metadata.carousel without a safe call (?.), which will lead to a crash.
Additionally, requireNotNull(metadata?.thumbnail) will throw an exception if metadata is null. This might not be the desired behavior if you want to handle getMetadata() failures gracefully.
Please restore the safe calls (?.) to prevent the application from crashing.
| requireNotNull(metadata?.thumbnail), | |
| metadata.banner ?: showJson.getAsJsonObject("boxartHighRes")!!.getAsString("url")!!.substringBefore("?"), | |
| metadata.carousel ?: showJson.getAsJsonObject("storyArt")!!.getAsString("url")!!.substringBefore("?"), | |
| requireNotNull(metadata?.thumbnail), | |
| metadata?.banner ?: showJson.getAsJsonObject("boxartHighRes")!!.getAsString("url")!!.substringBefore("?"), | |
| metadata?.carousel ?: showJson.getAsJsonObject("storyArt")!!.getAsString("url")!!.substringBefore("?"), |
| runCatching { CrunchyrollCachedWrapper.getEpisodesBySeriesId(countryCode.locale, episode.seriesId) }.getOrNull() | ||
| ?.sortedWith(compareBy({ it.episodeMetadata!!.seasonSequenceNumber }, { it.episodeMetadata!!.sequenceNumber })) | ||
| ?.lastOrNull { it.episodeMetadata!!.index() < episode.index() } | ||
| ?.id |
There was a problem hiding this comment.
With the addition of retrievePreviousEpisode in AbstractCrunchyrollWrapper, the logic here for finding the previous episode for Crunchyroll is now duplicated and overly complex. You can simplify the retrievePreviousEpisodes method in this file to directly call the new wrapper method.
This would make the code much cleaner and easier to maintain, which aligns with the goal of this pull request.
| runCatching { CrunchyrollCachedWrapper.getEpisodesBySeriesId(countryCode.locale, episode.seriesId) }.getOrNull() | ||
| ?.sortedWith(compareBy({ it.episodeMetadata!!.seasonSequenceNumber }, { it.episodeMetadata!!.sequenceNumber })) | ||
| ?.firstOrNull { it.episodeMetadata!!.index() > episode.index() } | ||
| ?.id |
There was a problem hiding this comment.
Similar to my comment on finding the previous episode, the logic here for finding the next episode is now duplicated since retrieveNextEpisode was added to AbstractCrunchyrollWrapper. You should refactor the retrieveNextEpisodes method in this file to use the new wrapper method for the Crunchyroll platform. This will remove redundant code and simplify this job.
| ) = NetflixWrapper.getEpisodesByShowId(key.countryCode, key.releaseDayPlatformSimulcast.name.toInt()) | ||
| .flatMap { video -> | ||
| val audioLocales = video.audioLocales.ifEmpty { key.netflixSimulcast.audioLocales } | ||
| audioLocales.map { audioLocale -> | ||
| convertEpisode( | ||
| key.countryCode, | ||
| key.netflixSimulcast.image, |
There was a problem hiding this comment.
The logic for handling audioLocales.ifEmpty and audioLocaleHasDelay has been removed from fetchApiContent. This seems to be a deliberate simplification as the corresponding configuration in NetflixConfiguration was also removed. However, this is a functional change. Previously, if episode.audioLocales was empty, it would default to key.netflixSimulcast.audioLocales (which itself defaulted to ja-JP). Now, if episode.audioLocales is empty, convertEpisode will return an empty list, and no episodes will be processed for that video. Please confirm if this change in behavior is intended.
| suspend fun retrievePreviousEpisode(locale: String, id: String): BrowseObject? { | ||
| val episode = runCatching { getEpisode(locale, id) }.getOrNull() ?: return null | ||
|
|
||
| // Fetch episodes by season and find the previous episode | ||
| runCatching { getEpisodesBySeasonId(locale, episode.seasonId) } | ||
| .getOrNull() | ||
| ?.sortedBy { it.sequenceNumber } | ||
| ?.firstOrNull { it.sequenceNumber < episode.sequenceNumber } | ||
| ?.let { return it.convertToBrowseObject() } | ||
|
|
||
| // Fetch episodes by series and find the previous episode | ||
| runCatching { getEpisodesBySeriesId(locale, episode.seriesId) } | ||
| .getOrNull() | ||
| ?.sortedWith(compareBy({ it.episodeMetadata!!.seasonSequenceNumber }, { it.episodeMetadata!!.sequenceNumber })) | ||
| ?.lastOrNull { it.episodeMetadata!!.index() < episode.index() } | ||
| ?.let { return it } | ||
|
|
||
| return null | ||
| } | ||
|
|
||
| suspend fun retrieveNextEpisode(locale: String, id: String): BrowseObject? { | ||
| // Fetch the current episode and check for nextEpisodeId | ||
| val episode = runCatching { getEpisode(locale, id) }.getOrNull() ?: return null | ||
| episode.nextEpisodeId?.let { return getObjects(locale, it).firstOrNull() } | ||
|
|
||
| // Fetch episodes by season and find the next episode | ||
| runCatching { getEpisodesBySeasonId(locale, episode.seasonId) } | ||
| .getOrNull() | ||
| ?.sortedBy { it.sequenceNumber } | ||
| ?.firstOrNull { it.sequenceNumber > episode.sequenceNumber } | ||
| ?.let { return it.convertToBrowseObject() } | ||
|
|
||
| // Fetch episodes by series and find the next episode | ||
| runCatching { getEpisodesBySeriesId(locale, episode.seriesId) } | ||
| .getOrNull() | ||
| ?.sortedWith(compareBy({ it.episodeMetadata!!.seasonSequenceNumber }, { it.episodeMetadata!!.sequenceNumber })) | ||
| ?.firstOrNull { it.episodeMetadata!!.index() > episode.index() } | ||
| ?.let { return it } | ||
|
|
||
| return null | ||
| } |
There was a problem hiding this comment.
These new helper methods retrievePreviousEpisode and retrieveNextEpisode are a great addition for centralizing logic. However, they don't seem to be used in UpdateEpisodeMappingJob.kt. The retrievePreviousEpisodes and retrieveNextEpisodes methods in that job still contain complex, duplicated logic for the Crunchyroll platform.
To complete the refactoring and cleanup, you should update UpdateEpisodeMappingJob.kt to use these new wrapper methods. This will significantly simplify the code in the job and remove the duplicated logic.
| suspend fun getPreviousEpisode(countryCode: CountryCode, showId: String, episodeId: String, checkAudioLocales: Boolean): Episode? { | ||
| val episodes = getEpisodesByShowId(countryCode, showId, checkAudioLocales) | ||
| val episodeIndex = episodes.indexOfFirst { it.id == episodeId } | ||
| require(episodeIndex != -1) { "Episode not found" } | ||
| return if (episodeIndex == 0) null else episodes[episodeIndex - 1] | ||
| } | ||
|
|
||
| suspend fun getNextEpisode(countryCode: CountryCode, showId: String, episodeId: String, checkAudioLocales: Boolean): Episode? { | ||
| val episodes = getEpisodesByShowId(countryCode, showId, checkAudioLocales) | ||
| val episodeIndex = episodes.indexOfFirst { it.id == episodeId } | ||
| require(episodeIndex != -1) { "Episode not found" } | ||
| return if (episodeIndex == episodes.size - 1) null else episodes[episodeIndex + 1] | ||
| } |
There was a problem hiding this comment.
These new helper methods getPreviousEpisode and getNextEpisode are useful. However, they are not currently used anywhere in the codebase. To make them effective, you should integrate them into UpdateEpisodeMappingJob.kt within the retrievePreviousEpisodes and retrieveNextEpisodes methods for the DISN platform. This would be a good opportunity to extend the functionality of that job to also find previous/next episodes for Disney+.
| suspend fun getPreviousEpisode(countryCode: CountryCode, showId: Int, episodeId: Int): Episode? { | ||
| val episodes = getEpisodesByShowId(countryCode, showId) | ||
| val episodeIndex = episodes.indexOfFirst { it.id == episodeId } | ||
| require(episodeIndex != -1) { "Episode not found" } | ||
| return if (episodeIndex == 0) null else episodes[episodeIndex - 1] | ||
| } | ||
|
|
||
| suspend fun getNextEpisode(countryCode: CountryCode, showId: Int, episodeId: Int): Episode? { | ||
| val episodes = getEpisodesByShowId(countryCode, showId) | ||
| val episodeIndex = episodes.indexOfFirst { it.id == episodeId } | ||
| require(episodeIndex != -1) { "Episode not found" } | ||
| return if (episodeIndex == episodes.size - 1) null else episodes[episodeIndex + 1] | ||
| } |
There was a problem hiding this comment.
Similar to the other wrappers, these new getPreviousEpisode and getNextEpisode methods are a good addition. However, they are currently unused. You should consider using them in UpdateEpisodeMappingJob.kt to handle finding previous/next episodes for the NETF platform, which would enhance the job's functionality.
Ziedelth
force-pushed
the
clean-up-code
branch
from
05da840 to
98790c8
Compare
No description provided.