1
\$\begingroup\$

I have been learning Ruby by reading books and tutorials, and practising some problems on Codewars to level-up my new skills, when I had an idea for a little Ruby code project. It will store programmes you like watching on the BBC iPlayer website, and list upcoming episodes, using an API (link is in 2nd line of code). For those unaware with iPlayer, it lets you watch most BBC TV programs, but only for a set time period after they were shown on TV. Sometimes I get behind in watching shows so in this example it will list episodes in order of soonest-to-expire, so I know what I need to watch first.

I decided to create a class which is an extension of a Hash, thinking that people can then do normal operations on it like they would a hash. I remain unsure if this is a good idea. Then there's a simple CLI to access it. I've written it to be extendable.

Two extensions I had in mind were:

  1. A GUI could use this class as-is
  2. A list of programs could be stored in an external file and loaded when the program is run.

So, let me know what you think. The main thing I thought of is error-trapping. It does a degree of assuming that the API will do what you expect it to.

class Iplayer_data < Array
    #for iPlayer API documentation, see http://mermade.github.io/swagger/index.html?url=https://raw.githubusercontent.com/Mermade/bbcparse/master/iblApi/swagger.json#!/Search/Search
    require 'httpclient' #I had difficulty installing a cUrl gem on windows, so I used this instead
    require 'JSON'
    attr_reader :categories, :regions

    def initialize
        @http = HTTPClient.new
        @lang = "en"
        @rights = "web"
        @version = "v1"
        # load categories and regions straight away, for future use, and so we have a local copy of the data
        @categories = load_standing_data("categories")
        @regions = load_standing_data("regions")
    end

    def i_search(query)
        url = "http://ibl.api.bbci.co.uk/ibl/#{@version}/search?q="
        url += URI.escape(query)
        url += "&lang=#{@en}&rights=#{@rights}&availability=available"
        get_http_content(url)["search"]["results"].each do |c|
            #don't bring back all the attributes. Basically just a test to see if I could do it
            #plus it's less data to display, which helps with testing.
            #there's no real good programmatic reason to limit what we bring back
            c.select! { |k,v| ["id", "title", "synopses"].include?(k) }
        end
    end

    def add_programme(name)
        if self.select {|x| x["title"].downcase==name.downcase}.count > 0
            return "Programme {name} is already in your list of programmes"
        end
        s = i_search(name)
        return "No program found with a name like that" if s.count==0
        if s[0]["title"].downcase == name.downcase
            self << s[0]; "Successfully added #{s[0]['title']}"
        else
            "Could not find a program of the name #{name}. Did you mean #{s[0]['title']}?"
        end
    end

    def delete_programme(name)
        if self.select {|x| x["title"].downcase==name.downcase}.count == 0
            return "Programme #{name} is not in your list of programmes"
        end
        self.delete_if {|x| x["title"].downcase==name.downcase}
        "#{name} has been deleted"
    end

    def get_episodes
        episodes = []
        self.each do |programme|
            pid = programme["id"]
            url = "http://ibl.api.bbci.co.uk/ibl/#{@version}/programmes/#{pid}/episodes?rights=#{@rights}"
            url += "&availability=available&initial_child_count=2&per_page=200"
            episodes += get_http_content(url)["programme_episodes"]["elements"]
        end
        episodes
    end

    def get_http_content(url);JSON.parse(@http.get(url).content);end

    private
    #categories and regions have pretty much the same API so we can load them both through 1 method
    def load_standing_data(name)
        raw = get_http_content("http://ibl.api.bbci.co.uk/ibl/#{@version}/#{name}?lang=#{@lang}")
        # from an array of hashes, return a hash of hashes, with the id field as the key
        output = Hash.new
        raw["#{name}"].each { |x| output[x["id"]] =  x }
        output
    end
end

##################### END OF CLASS #####################
###### NOW CREATE A SIMPLE COMMAND-LINE INTERFACE ######

def display_help
    puts 'Iplayer_data helps you keep track of your favourite programs.'
    puts 'It can store the programs you want to follow, then display information regarding episodes of those programs.'
    puts 'You can (a)dd, (d)elete or (v)iew your favourite programs...'
    puts 'Display (e)pisodes, (c)ategories, or (r)egions, or e(x)it.'
    puts 'Some instructions require extra input - just put a space after the first letter than type it in.'
    puts 'eg to add Top Gear, input "a top gear" (case insensitive)'
    #(d)elete, (p)rogrammes
end

require 'pp' # pretty print

ip = Iplayer_data.new
re=''
until re[0] == 'x' do

    print '>'
    re = gets.chomp
    case re[0]
        when '?' then display_help
        when 'a' then puts ip.add_programme(re[2..-1])
        when 'd' then puts ip.delete_programme(re[2..-1])
        when 'v' then pp ip
        when 'c' then pp ip.categories
        when 'r' then pp ip.regions
        when 'e' then
            #list of episodes, ordered by soonest to latest to expire
            ep = ip.get_episodes.sort_by { |episode| episode["versions"][0]["availability"]["end"]}
            ep.each do |e|
                e["time_left"] = e["versions"][0]["availability"]["remaining"]["text"]
                puts e.select {|k,v| ["id", "title", "subtitle", "time_left"].include?(k) }
            end
            puts "No episodes found" if ep.count==0
        when 'x' then puts "Exiting..."
        else puts "Sorry, I did not recognize that input. Type \"?\" for help."
    end
end
\$\endgroup\$
2
  • 1
    \$\begingroup\$ This would have been a comment but for the requirement for 50+ reputation. I can't comment on the Ruby code quality, but as the author of the iPlayer API documentation I'm extremely pleased to see it used like this. If you have any questions regarding the API itself, please don't hesitate to raise an issue on GitHub. \$\endgroup\$ Commented Aug 2, 2016 at 16:07
  • \$\begingroup\$ Thanks! I did have one or two queries so I will raise them on GitHub at some point... \$\endgroup\$ Commented Aug 2, 2016 at 18:22

1 Answer 1

3
\$\begingroup\$

My thoughts, in no particular order:

  1. Fix your indentation. Ruby uses 2 spaces of indentation; no more, no less, and not tabs

  2. requires should generally be file-level, not class-level

  3. Don't name a class with an underscore. Classes are PascalCase in Ruby. Besides, the _data suffix doesn't make much sense; if anything the class is an "iPlayer client" and should be named something like that.

  4. Why derive the class from Array? That's crazy overkill compared to just having a programs (or programmes) array as an instance variable. Besides, if it's an array instance, I can do weird things like ip << "foobar", which doesn't make much sense. If you do want an array-like API for the class, implement an each method, and include Enumerable for cleaner code.

  5. Model the domain. You should probably create a separate Programme class, which can have an episodes method. Would be cleaner than having one class both deal with programs and episodes, and just loading all episodes for everything. It'd might be good to subdivide further into a pure API client class, which can be used to make API calls by a "programs manager" class, a "program" class, and perhaps even an "episode" class.

  6. You have some instance variables that should probably be constants since that's how they're used (@lang, @rights)

  7. You repeat most of the URL in several places. Make the base URL a constant.

  8. Make a method for generating URLs instead of glueing together bits of string in multiple places. E.g.

    def url_for(path, params = {})
      query = URI.encode_www_form(params)
      "#{BASE_URL}#{path}?#{query}"
    end
    
    # Example
    url_for('search', q: 'top gear', rights: 'web', availability: 'available', lang: 'en')
    # => http://ibl.api.bbci.co.uk/ibl/v1/search?q=top+gear&rights...
    

    Can't remember if an HTTPClient instance can be configured to handle some of this too (e.g. a common base URL). It might.

  9. Don't do this:

    output = Hash.new
    raw["#{name}"].each { |x| output[x["id"]] =  x }
    output
    

    Don't create a closure variable and manipulate it from inside a block.
    Don't write ["#{name}"] when [name] has the same result.

    Do something like this instead:

    raw[name].map { |item| [item['id'], item] }.to_h
    
  10. If categories and regions aren't necessarily used for anything, don't eagerly load them. Instead load them lazily, when needed, and memoize:

    def categories
      @categories ||= load_standing_data('categories')
    end
    

    Incidentally, load_standing_data is an odd name... not terribly explanatory.

\$\endgroup\$
6
  • \$\begingroup\$ Thanks - definitely more I agree with here than disagree with. Much to ponder on for the future. \$\endgroup\$ Commented Aug 1, 2016 at 21:59
  • \$\begingroup\$ @WelshGandalf Have to ask: Which parts do you disagree with? \$\endgroup\$
    – Flambino
    Commented Aug 1, 2016 at 22:06
  • \$\begingroup\$ 1. I set tabs to be 2 spaces in my editor which is I've read is OK? - though I completely understand you pointing it out here. 3. I agree mostly, but iplayer client isn't the best name either as you can't actually play iplayer programmes through it, 6. they can be different values and eventually I will initialise them when calling the class rather than setting them to fixed values. I suppose therefore I agree with the spirit of everything you say if not the specifics :) I think 4,5,8,9 and 10 are the ones that will help me improve the most (I've never come across a hash as a parameter before) \$\endgroup\$ Commented Aug 2, 2016 at 9:47
  • \$\begingroup\$ Ah, right. I think you may have set your editor to display tabs as 2 spaces wide - but it's still writing tab characters (\t). Ruby, however, conventionally uses two spaces - as in two individual space characters - instead of tab characters. You should be able to configure your editor to use "soft tabs", so that when you press the tab key, it actually types spaces instead of a tab character (and treats those two spaces as a single thing, so deleting works normally etc.) \$\endgroup\$
    – Flambino
    Commented Aug 2, 2016 at 10:42
  • \$\begingroup\$ @WelshGandalf (cont'd) 3. Well, true it's not a video player. Still, client is more accurate than data so some variation on that might work. 6. Yeah, I imagine you might want to make those values configurable - but cross that bridge when you come to it. When you do, I'd advise using getter methods for the values rather than "raw" instance vars. Rely on the abstraction/indirection of a method, rather than the concrete value. This makes your code more flexible. \$\endgroup\$
    – Flambino
    Commented Aug 2, 2016 at 11:37

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.