search

amugofjava / podcast_search

A simple library providing programmatic access to the iTunes search API for podcasts.
MIT License
43 stars 26 forks source link

added optional genre filtering to top charts function #2

Closed kbanta11 closed 4 years ago

kbanta11 commented 4 years ago

I added the ability to pass in a genre ID to the charts function so that you can get the top podcasts by genre.

amugofjava commented 4 years ago

Hi Kbanta11,

Thanks for the pull request. I've tested it and it works great, but there are a couple of issues. You have added the genre parameter as an plain int and pass this on to the iTunes API. This works fine, but it doesn't help the user know which ID relates to which genre. I think it would be better to implement it in a similar way to the Country parameter. With that, you essentially have an enum that represents the Country code so the end user passes Country.UNITED_KINGDOM rather than 'UK'. This not only makes it clearer to the end user but also ensures the user can only specify a valid value. iTunes does provide a list of genre values. It's very large, but I would think just picking out the primary genres (excluding the sub-genres) would by fine. You could then have say Genre.SPORTS rather than 1512.

http://itunes.apple.com/WebObjects/MZStoreServices.woa/ws/genres

Secondly, if you use an IDE such as Android Studio, it's a great idea to check the Dart analysis window before committing. It's a helpful tool that points out certain syntax or formatting errors. In the search.dart file your if statement is split over two lines, but does not have any {} brackets. The Effective Dart style guide specifies that you either need to use curly brackets or make your if statement one line. My personal preference is to always use {}. The style guide is a useful read (in fact, reading it again now I'm not sure my constants follow the guide!) and can be found here:

https://dart.dev/guides/language/effective-dart/style#do-use-curly-braces-for-all-flow-control-structures.

Thanks,

Ben.

kbanta11 commented 4 years ago

I've made some of your suggested changes above, mainly adding to your existing Genre model (which didn't appear to be in use anywhere?) to have constructors for the top level Apple Podcasts genres (I haven't included the mappings for the sub-genres yet, only top-level genres). It's basically just a mapping from an "English" genre name to the ID to pass into the API call.

Let me know your thoughts!

amugofjava commented 4 years ago

Thanks for the changes, it fits in nicely with the other filters and works really well. Thank you for your contribution.