From: "Jesús Gabriel y Galán" <jgabrielygalan@...>
Date: 2010-03-31T16:47:33+09:00
Subject: Re: RubyDictionary - First Try

On Wed, Mar 31, 2010 at 12:40 AM, Max Schmidt
<max.schmidt.privat@googlemail.com> wrote:
> 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.