rubynuby code critique request

[Jeff Dickens]

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)
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
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
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

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

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


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
$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"
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
outFile = File.new(vars[:wordIndexFile], "w")
$songs.wordindex(outFile)
outFile.close
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

 

[T. Onoma]

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.

I think you're right, this is a little more than the intended use of the Wiki
NubyCodeCritique ;-)

Well, I'd say you've done dang well. I only noticed a few places you could be
more concise, but nothing of major significance. At least as far as I can
tell from just looking it over briefly. It works I take it? That's the most
important thing! Beyond that you may wish to explore a bit of unit
testing.

A few quick spot suggestions:

You might want to put more of the string cleanup in the parsing of the file
rather than the append method. Also in parsing a file like this all \s+
spaces can be reduced to a single space (as it stands it looks like you'll
still retain multiple spaces in between strings). So try .gsub(/\s+/,' ') on
the file as it is read in.

It seems like you prefer "word" methods, but it is possible to do stuff like:

class ArtistIndex
def initialize
@aindex = Hash.new(nil)
end
# instead of def aindex (anObject, artist)
def []=(anObject, artist)
@aindex[artist] = [] if @aindex[artist].nil?
@aindex[artist].push(anObject)
end
# instead of def alookup(artist)
def [](artist)
@aindex[artist]
end
# if it were me i'd just call this #artists
def listartists
@aindex.keys.sort
end
end

These make it work like a Hash, which it essentially is. In fact you could
subclass Hash:

class ArtistIndex < Hash
...
def listartists
self.keys.sort
end
end

And then you'd get all the abilites of hash as well, though this might be more
than you need/want.

My two bits, HTH,
T.

 

[Robert Klemme]

Newsbeitrag

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