Hello folks,
Last week I started learning Ruby by the book The Ruby Programming
Language if it's relevant. Today I decided to practise a little bit with
my new knowledge by trying to develop my first "serious" application in
Ruby.
Welcome to Ruby and very good job. You achieved very nice things.
The idea was to create a small virtual dictionary based on CLI
with a few self-explaining functions like inserting/removing/search for
a data set or save/load it in a *.txt file.
One question: when I think dictionary, I think this:
http://en.wikipedia.org/wiki/Associative_array, in which what is
unique is the keys, not the pairs. Your implementation allows to
insert two pairs with the same key:
irb(main):159:0* d = Dictionary.new
=> #<Dictionary:0xb7be8db0 @entries=[]>
irb(main):161:0> d.insert_words "aa", "bb"
=> [#<DictionaryEntry:0xb7bdd8fc @word2="bb", @word1="aa">]
irb(main):162:0> d.insert_words "aa", "cc"
=> [#<DictionaryEntry:0xb7bdd8fc @word2="bb", @word1="aa">,
#<DictionaryEntry:0xb7bd6494 @word2="cc", @word1="aa">]
and this looks a bit strange. But, anyway, I'll continue with the
assumption that this is what you really want.
If this is not the case, and you want unique keys, you will have to
change many things.
Since this is my first attempt to a script language I wanted to know
what you think of my program. Are there any passages which could be
written more efficient in performance? In general, is the coding
style/commenting OK? What can I improve regarding OOP? Furthermore,
there are some commented in-code questions on line 26,93 and 99 - can
you answer them maybe?
Some comments, mostly idiomatic:
1.-
def initialize(entries=nil)
if entries==nil
@entries = Array.new
else
@entries = entries
end
end
this could be done like this:
def initialize(entries=nil)
@entries = entries || [] # or Array.new if you prefer
end
2.- def insert_words: I would call the method:
def []=(word1, word2) which allows you to say d["word"] = "second word"
Nice syntactic sugar, although this goes more in line of what I said
about the dictionary being a key-value set.
3.-
def self.is_word? word
if (word.match(/[a-zA-Z]{2,15}/)[0].length == word.length) then true
else false
end
# (word.match(/[a-zA-Z]{2,15}/)[0].length == word.length)
# easier way?
end
Why don't you allow one letter words? There's a problem with this
implementation though: if the string doesn't match you get a
irb(main):160:0> d.insert_words "a", "b"
NoMethodError: undefined method `[]' for nil:NilClass
from (irb):94:in `is_word?'
because match will return nil, and you are calling [0] on that.
def self.is_word? word
word.match(/\A[:alpha:][:alpha:]+\z/) #you can remove the first
[:alpha:] to allow 1-letter words
end
4.- def insert_dict_entry(entry)
insert_words(entry.word1, entry.word2) # any better solution?
end
Another way, not necesarily better:
def insert_dict_entry(entry)
@entries << entry.dup
end
5.- def insert_array(words1, words2)
# arrays have to have same length
raise ArgumentError, "Invalid array arguments (not same length)"
unless words1.length==words2.length
# iterate on words1 and try to insert every pair
words1.each_index {|x| insert_words(words1[x], words2[x])}
end
I would do:
def insert_array(words1, words2)
words1.zip(words2).each {|first,second| insert_words(first,second)
if first && second}
end
this way, even if the arrays have different size, you store as much as
you can. If you still want the check you can add it.
6.- def remove(r_word1, r_word2)
def remove(word1, word2)
@entries.delete_if {|entry| entry == [word1,word2]} # if you keep
the enumerable equality check, see below
end
7.- def remove_at(index)
This one doesn't make sense for the public interface of a dictionary.
If you implement remove as above, you don't need it anymore.
8.- def search s_word
def search word
@entries.select {|entry| entry.word1.include?(word) ||
entry.word2.include?(word)}
end
9.- def has_word_pair?(word1, word2)
Why do you need the index as a return value?
def has_word_pair?(word1,word2)
@entries.find {|entry| entry == [word1,word2]}
end
10.- to_s
def to_s
s = "Current state of Dictionary (#{self.object_id})\n"
@entries.each_with_index {|entry, i| s << sprintf("%3d %15s |
%15s\n", i, entry.word1 , entry.word2)
s
end
DictionaryEntry
When you create a dictionary entry, you might want to dup the words to
avoid aliasing:
def initialize (word1, word2)
@word1 = word1.dup unless word1.frozen?
@word2 = word2.dup unless word2.frozen?
end
11.-
def ==(other)
if (other.is_a?(Enumerable) && @word1 == other[0] && @word2 == other[1])
true
elsif (other.is_a?(DictionaryEntry) && @word1 == other.word1 &&
@word2 == other.word2) # not tested
puts "dict entry"
true
else
false
end
end
Enumerable doesn't provide the method [], so if you are allowing a
equality test against an Enumerable you should only use Enumerable
methods. For example:
def ==(other)
case other
when Enumerable
@word1 == other.to_a[0] && @word2 == other.to_a[1]
when DictionaryEntry
@word1 == other.word1 && @word2 == other.word2
else
false
end
end
I'm not sure you want to keep the equality comparison with an
Enumerable, because I think you added it more as a convenience than as
a real requirement. If you remove it, some of my suggestions above
should be reviewed.
I don't have time now to comment on the CLI interface, but I think
that could be refactored a little bit too.
Jesus.
