1
\$\begingroup\$

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
\$\endgroup\$
1
  • \$\begingroup\$ 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. \$\endgroup\$ Commented Jan 29, 2016 at 10:22

1 Answer 1

1
\$\begingroup\$
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
\$\endgroup\$
5
  • \$\begingroup\$ I find unless v.nil? then %Q["#{v}"] end a bit hard to understand, do you think a ternary could be better? \$\endgroup\$ Commented Jan 29, 2016 at 19:25
  • \$\begingroup\$ 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. \$\endgroup\$ Commented Jan 29, 2016 at 22:07
  • \$\begingroup\$ 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/… \$\endgroup\$ Commented Jan 29, 2016 at 22:08
  • \$\begingroup\$ 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. \$\endgroup\$ Commented Jan 29, 2016 at 22:11
  • \$\begingroup\$ @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? \$\endgroup\$ Commented Jan 30, 2016 at 3:25

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.