Creating a dictionary

Question

I have a Ruby programming task. My code passes all the rspec requirements, but I am not sure if I've done it in the right way. And I'm not quite sure why I need :entries there...

Here is the rspec test:

# # Topics
#
# * Hash
# * Array
# * instance variables
# * regular expressions
require '11_dictionary'
describe Dictionary do
 before do
 @d = Dictionary.new
 end
 it "is empty when created" do
 expect(@d.entries).to eq({})
 end
 it "can add whole entries with keyword and definition" do
 @d.add("fish" => "aquatic animal")
 expect(@d.entries).to eq({"fish" => "aquatic animal"})
 expect(@d.keywords).to eq(["fish"])
 end
 it "add keywords (without definition)" do
 @d.add("fish")
 expect(@d.entries).to eq({"fish" => nil})
 expect(@d.keywords).to eq(["fish"])
 end
 it "can check whether a given keyword exists" do
 expect(@d.include?("fish")).to be_falsey
 end
 it "doesn't cheat when checking whether a given keyword exists" do
 expect(@d.include?("fish")).to be_falsey # if the method is empty, this test passes with nil returned
 @d.add("fish")
 expect(@d.include?("fish")).to be_truthy # confirms that it actually checks
 expect(@d.include?("bird")).to be_falsey # confirms not always returning true after add
 end
 it "doesn't include a prefix that wasn't added as a word in and of itself" do
 @d.add("fish")
 expect(@d.include?("fi")).to be_falsey
 end
 it "doesn't find a word in empty dictionary" do
 expect(@d.find("fi")).to be_empty # {}
 end
 it "finds nothing if the prefix matches nothing" do
 @d.add("fiend")
 @d.add("great")
 expect(@d.find("nothing")).to be_empty
 end
 it "finds an entry" do
 @d.add("fish" => "aquatic animal")
 expect(@d.find("fish")).to eq({"fish" => "aquatic animal"})
 end
 it "finds multiple matches from a prefix and returns the entire entry (keyword + definition)" do
 @d.add("fish" => "aquatic animal")
 @d.add("fiend" => "wicked person")
 @d.add("great" => "remarkable")
 expect(@d.find("fi")).to eq({"fish" => "aquatic animal", "fiend" => "wicked person"})
 end
 it "lists keywords alphabetically" do
 @d.add("zebra" => "African land animal with stripes")
 @d.add("fish" => "aquatic animal")
 @d.add("apple" => "fruit")
 expect(@d.keywords).to eq(%w(apple fish zebra))
 end
 it "can produce printable output like so: [keyword] 'definition'" do
 @d.add("zebra" => "African land animal with stripes")
 @d.add("fish" => "aquatic animal")
 @d.add("apple" => "fruit")
 expect(@d.printable).to eq(%Q{[apple] "fruit"\n[fish] "aquatic animal"\n[zebra] "African land animal with stripes"})
 end
end

Here is my code:

class Dictionary
 attr_accessor :entries
 def initialize
 @hash={}
 end
 def add(a)
 if a.class == Hash
 a.each_pair { |k, v|
 @hash[k]=v
 }
 else
 @hash[a]=nil 
 end
 end
 def entries
 @hash
 end
 def keywords
 @hash.keys.sort
 end
 def include?(k)
 @hash.key?(k)
 end
 def printable
 str=[]
 [email protected]
 key.each do |el|
 @hash.each_pair do |k,v|
 if k==el
 str << "[#{k}] \"#{v}\""
 end
 end
 end
 return str.join("\n")
 end
 def find(str)
 if str.class == Hash
 str.each_pair { |k, v|
 return (@hash.select { |k,v| k.include?(k)})
 }
 elsif str.class == String 
 return (@hash.select {|k,v| k.include?(str)})
 end
 end
end

• Simon Forsberg: For future reference, you might want to look at recommendations for posting a good quality question. I would have liked to see a bit of a summary of what your code does and the functionality it provides.

Answer 1

class Dictionary
 attr_accessor :entries

Indentation in Ruby is two spaces, not four. You have that correct in your specs, but not in the application code.

Also, you create here a pair of methods, entries= which you never use anywhere, and entries, which you overwrite further down, anyway. Either get rid of this line, or (preferred!) actually make use of those methods.

 def initialize
 @hash={}

There should be a space on both sides of the = sign. This applies to operators in general.

[Note: there are some style guides that advocate not using spaces around the = sign for optional parameters with default arguments, but a) this is not universally accepted and b) not the case here anyway.]

 end
 def add(a)

This should probably be called << instead.

[Note: I realize, this is prescribed by the spec. IMO, the spec should be changed.]

 if a.class == Hash

You should never compare the class of an object like this. Ideally, you should use runtime ad-hoc polymorphism. If that isn't possible, at least use Object#is_a?, Object#kind_of?, or Module#===.

 a.each_pair { |k, v|
 @hash[k]=v
 }

There are two different camps when it comes to block styles: one camp says to use {/} for one-liners and do/end for multiple lines. The other camp says to use {/} for "functional" blocks and do/end for side-effecting blocks.

Regardless of which camp you follow: your block has three lines and side-effects, so it should use the keyword form.

 else
 @hash[a]=nil 
 end
 end

There should be an empty between the two method definitions.

 def entries
 @hash
 end
 def keywords
 @hash.keys.sort
 end
 def include?(k)
 @hash.key?(k)
 end
 def printable

This should be called to_s. to_s is the standard name for a method that converts an object into a textual representation. It is used automatically by methods such as Array#join or Kernel#puts.

[Note: I realize, this is prescribed by the spec. IMO, the spec should be changed.]

 str=[]
 [email protected]
 key.each do |el|
 @hash.each_pair do |k,v|
 if k==el
 str << "[#{k}] \"#{v}\""

You could save yourself some hassle here by using something else than " to delimit the string.

This pattern, where you initialize an accumulator to an "empty" value and then iterate over a collection while appending to the accumulator at every iteration, is called a Catamorphism, fold, or reduce. In Ruby, it is known as inject.

However, in this case, it can also be done even simpler, with a map.

 end
 end
 end
 return str.join("\n")

The return is superfluous here, the return value of a method is the value of the last expression which is evaluated inside the method body.

 end
 def find(str)
 if str.class == Hash
 str.each_pair { |k, v|
 return (@hash.select { |k,v| k.include?(k)})
 }

There is nothing in the spec about passing Hashes to find. In fact, you named your parameter str clearly indicating that you expect it to be a String. Then why even bother checking for Hashes?

 elsif str.class == String 
 return (@hash.select {|k,v| k.include?(str)})

Here you check whether the key includes the search string. However, from the documentation in the spec, it appears that the search string should be a prefix of the key. Unfortunately, there is actually no test in the spec to test for that distinction.

 end
 end
end

This is what it would look like with those suggestions applied:

class Dictionary
 attr_reader :entries
 def initialize
 self.entries = {}
 end
 def add(a)
 case a
 when Hash then entries.merge!(a)
 else entries[a] = nil
 end
 end
 def keywords
 entries.keys.sort
 end
 def include?(k)
 entries.key?(k)
 end
 def printable
 entries.
 sort.
 map {|k, v| ["[#{k}]", unless v.nil? then %Q["#{v}"] end] }.
 map(&:compact).
 map {|kvp| kvp.join(' ') }.
 join("\n")
 end
 def find(key)
 entries.select {|k, v| k.start_with?(key) }
 end
 private
 attr_writer :entries
end

• Caridorc: I find unless v.nil? then %Q["#{v}"] end a bit hard to understand, do you think a ternary could be better?
• Jörg W Mittag: I don't like the conditional operator very much. I strongly prefer a conditional expression. I went back and forth between the positive and the negative formulation, though: unless v.nil? then %Q["#{v}"] end vs. if v.nil? then nil else %Q["#{v}"] end. In the end, I decided against the positive formulation, because even though the condition is easier to read, the "interesting" stuff happens in the else branch. In the negative formulation, the interesting stuff happens in the then branch, and, even better, the non-interesting stuff allows us to leave out the else altogether.
• Jörg W Mittag: All in all, I don't believe v.nil? ? nil : %Q["#{v}"] will buy much over if v.nil? then nil else %Q["#{v}"] end. See also this comment of mine on another answer for some of my thoughts on the conditional operator: codereview.stackexchange.com/questions/116236/…
• Jörg W Mittag: Honestly, I find the whole idea of a dictionary with incomplete entries just weird, and the weirdness in the to_s is actually just a symptom of that.
• user3907934: @JörgWMittag Thank you very much for your feedback. I will take into consideration some of your suggestions. My main question, however, remains open. Does my code do the stuff that I am suppose to learn according to the spec?