From: Robert Klemme <bob.news@...>
Date: 2003-12-08T21:37:07+09:00
Subject: Re: rubynuby code critique request



"Jeff Dickens" <dickens@dreamgeardotcom.butdontspamme> schrieb im
Newsbeitrag
news:aVaAb.156105$Ec1.6116265@bgtnsc05-news.ops.worldnet.att.net...
> I tried using the wiki at rubygarden.org but it just made a mess...
probably
> not designed for things this big.
>
> Anyhow, here's a (longish) program I've written as my first "real" ruby
> program.  I'm reading from the "Programming Ruby" book at
rubycentral.com,
> and what do you know, I actually do have a jukebox of sorts.  Actually
it's
> a radio station I do some work for.  So I expanded the example code
some.
> Have a look and poke some holes.
>
> It reads a file that looks like this:
>
>   18 And Life; Skid Row
>   19th Nervous Breakdown; Rolling Stones
>   2000 Light Years From Home; Rolling Stones
>   20th Centry Fox; Doors
>   2112 (Parts 1-7); Rush
>   29 Palms; Robert Plant
>   30 Days In The Hole; Humble Pie
>   5:15; Who
>   867-5309; Tommy Tutone
>   #9 Dream; John Lennon
>   Abacab; Genesis
>   Adam Raised a Cain; Bruce Springsteen
>   A Day In The Life; Beatles
>   A Face In The Crowd; Tom Petty
>   After Midnight; Eric Clapton
>   After The Gold Rush; Neil Young
>   Against The Wind; Bob Seger
>   A Hard Day's Night; Beatles
>
>
> It reads the file and can create a word list that could be used for a
> full-text search, a listing by artist, and a listing by artist in HTML
with
> a clickable index.
>
> Here's the code.  Your input is appreciated.
>
> #!/usr/local/bin/ruby -w
>
> require 'optparse'
>
>
> class SongList
>   def initialize
>     @songs = Array.new
>     @index = WordIndex.new
>     @aindex = ArtistIndex.new
>   end
>   def append(aSong)

Formatting the names is not a responsibility of the list.  This should be
done either in the Song or during reading.

>     aSong.name.sub!(/\s+$/,'')    # lose leading and trailing
>     aSong.artist.sub!(/\s+$/,'')  # whitespace
>     aSong.name.sub!(/^\s+/,'')
>     aSong.artist.sub!(/^\s+/,'')


>     @songs.push(aSong)
>     @index.index(aSong, aSong.name, aSong.artist)
>     @aindex.aindex(aSong, aSong.artist)
>     self
>   end
>   def [](key)
>     return @songs[key] if key.kind_of?(Integer)
>     return @songs.find { |aSong| aSong.name == key }
>   end

Can be delegated to the indexes:
>   def lookup(aWord)
>     @index.lookup(aWord)
>   end
>   def wordlist
>     @index.wordlist
>   end

>   def wordindex(outFile)
>     @index.wordlist.each do |aWord|
>       outFile.puts aWord
>       @index.lookup(aWord).each do |aSong|
>         outFile.puts "\t" + aSong.name + "; " + aSong.artist
>       end
>     end
>   end

Can be delegated to the indexes:
>   def alookup(artist)
>     @aindex.alookup(artist)
>   end
>   def listartists
>     @aindex.listartists
>   end

>   def listsongs(outFile)
>     @aindex.listartists.each do |anArtist|
>       outFile.puts anArtist
>       @aindex.alookup(anArtist).sort \
>       {|a,b| a.name <=> b.name}.each do |aSong|
>         outFile.puts "\t" + aSong.name
>       end
>     end
>   end

I'd put the HTML methods into antoher class because this is view specific,
i.e. you might want to do other presentations as well:
>   def html(outFile)
>     outFile.print "<a name=\"top\" id=\"top\"></a>\n"
>     @aindex.listartists.each do |anArtist|
>       safeKey = anArtist.gsub(/[\s\.\-\,\/\\\']/, '')
>       outFile.print "<a href=\"##{safeKey}\">"
>       outFile.print "<div class=\"artistx\">#{anArtist}</div></a>\n"
>     end
>     @aindex.listartists.each do |anArtist|
>       safeKey = anArtist.gsub(/[\s\.\-\,\/\\\']/, '')
>       htmlDetail(outFile, anArtist, safeKey)
>     end
>   end
>
>   def htmlDetail(outFile, anArtist, safeKey)
>     outFile.print "<div class=\"artist\">"
>     outFile.print "<a name=\"#{safeKey}\" id=\"#{safeKey}\">"
>     outFile.print "#{anArtist}</a>\n"
>     outFile.print "<a href=\"#top\" class=\"toplink\">top</a></div>\n"
>     @aindex.alookup(anArtist).sort \
>     {|a,b| a.name <=> b.name}.each do |aSong|
>       outFile.print "\t<div class=\"song\">"
>       outFile.print "#{aSong.name}</div>\n"
>     end
>   end
>
> end #class SongList

Index classes are not needed when using Hash's features:
> class WordIndex
>   def initialize
>     @index = Hash.new(nil)
>   end
>   def index(anObject, *phrases)
>     phrases.each do |aPhrase|
>       aPhrase.scan(/\w[-\w]+/) do |aWord|   # extract each word
>         aWord.downcase!
>         @index[aWord] = [] if @index[aWord].nil?
>         @index[aWord].push(anObject)
>       end
>     end
>   end
>   def lookup(aWord)
>     @index[aWord.downcase]
>   end
>   def wordlist
>     @index.keys.sort
>   end
> end
>
> class ArtistIndex
>   def initialize
>     @aindex = Hash.new(nil)
>   end
>   def aindex (anObject, artist)
>     @aindex[artist] = [] if @aindex[artist].nil?
>     @aindex[artist].push(anObject)
>   end
>   def alookup(artist)
>     @aindex[artist]
>   end
>   def listartists
>     @aindex.keys.sort
>   end
> end
>

Ok.
> class Song
>   def initialize(name, artist)
>     @name = name
>     @artist = artist
>   end
>   attr_reader :name, :artist
>   def to_s
>     "Song: #{@name}--#{@artist}"
>   end
> end
>
> def build_data(inFile); # Read the data and build structures

# don't use a global var here, just return the SongList
>   $songs = SongList.new # inFile is an open file descriptor
>   inFile.each do |line|
>     name, artist = line.chomp.split(/\s*;\s*/)
>     unless name and artist
>       puts "input data format error"

better use an exception here and don't terminate the whole script:
>       exit
>     end
>     $songs.append Song.new(name, artist)
>   end
> end
>
>
> #---------------Main Begins Here--------------------------
>
>
> $0 = File.basename($0)
>
> vars = {}
> ARGV.options do
>   |opts|
>   opts.banner = "\nUsage: #{$0} [options] [input filename]\n\n" \
>   "Reads from stdin if [input filename] is \n" \
>   "not present.\n\n" \
>   "Data format is \"Song Title; Artist\"\n\n"
>
>   opts.on_head("Options:")
>
>   opts.on("-w", "--wordindex WORDINDEXFILE", String,
>    "Generate Word Index",
>    "in WORDINDEXFILE") {|vars[:wordIndexFile]|}
>
>   opts.on("-g", "--generatehtml HTMLFILE", String,
>    "Generate HTML File",
>    "in HTMLFILE") {|vars[:htmlFile]|}
>
>   opts.on("-l", "--list LISTFILE", String,
>    "Generate List File",
>    "in LISTFILE") {|vars[:listFile]|}
>
>   opts.on_tail("-h", "--help", "show this message") {puts opts; exit}
>
>   opts.parse!
>   end
>
> if vars.length < 1
>   puts "Nothing to do"
>   puts ARGV.options
>   exit
> end
>
> if ARGV.length > 1
>   puts "Too many arguments"
>   exit
> end
>
> if ARGV.length == 1
>   then
>   unless FileTest.readable?(ARGV[0]) # test if input file exists
>     puts ARGV[0] + " not readable"
>     exit
>   end
>   inFile=File.open(ARGV[0])  # open inputfile
>   build_data(inFile)  # and read/build data
>   inFile.close
>   else
>   build_data($stdin)  # just use stdin if no input file
> end
>
> if vars[:wordIndexFile]  # Generate word index file if -w

better use block form, it's safer:
>   outFile = File.new(vars[:wordIndexFile], "w")
>   $songs.wordindex(outFile)
>   outFile.close

File.open(vars[:wordIndexFile], "w") do |outFile|
  $songs.wordindex(outFile)
end

> end
>
> if vars[:listFile]  # Generate listing file if -l
>   outFile = File.new(vars[:listFile], "w")
>   $songs.listsongs(outFile)
>   outFile.close
> end
>
> if vars[:htmlFile]  # Generate html file if -g
>   outFile = File.new(vars[:htmlFile], "w")
>   $songs.html(outFile)
>   outFile.close
> end

That's how I would start as a quick solution.  Disadvantages:

 - it's not complete

require 'set'

# special class to prevent lookups from creating entries
class Index < Hash
  def insert(key, value)
    ( self[key] ||= Set.new ) << value
  end
end

class Song
  attr_accessor :artist, :title

  def initialize(title, artist)
    @artist, @title = artist, title
  end

  def eql_impl?( s )
    title == s.sitle && artist == s.artist
  end

  alias :eql? :eql_impl?
  alias :== :eql_impl?

  def hash
    title.hash ^ artist.hash
  end

  private :eql_impl?
end


class SongList
  def create_index
    # Hash.new {|h,k|h[k]=Set.new}
    Index.new
  end

  attr_reader :artist_index, :word_index, :songs

  def initialize
    @artist_index = create_index
    @word_index = create_index
    @songs = Set.new
  end

  def add(song)
    songs << song
    artist_index.insert( song.artist, song )

    [song.artist, song.title].each do |str|
      str.scan( /\w+/ ) {|word| word_index.insert( word, song ) }
    end
  end

  def load(file)
    File.open( file ) do |io|
      io.each_line do |line|
        if /^\s*\b(.*)\b\s*;\s*\b(.*)\b\s*$/ =~ line
          add Song.new($1, $2)
        end
      end
    end
  end

  def self.load(file)
    sl = self.new
    sl.load( file )
    sl
  end
end

# simple test
sl = SongList.load( "songs.txt" )
p sl
p sl.word_index[ "A" ]


Kind regards

    robert