4
\$\begingroup\$

I'm new to Ruby and don't know all the methods, so I was thinking there would probably be and easier way to do something like this. I'd love any input on how to refactor this.

   define_singleton_method(:delete) do |id|
        revised_words = []
        @@words.each do |word|
          if word.id() != id
            revised_words.push(word)
          else
            Definition.delete_by_word_id(id)
          end
        end
        @@words = revised_words
      end
\$\endgroup\$
3
  • \$\begingroup\$ Please provide a more specific title. "Delete method" sounds too generic. \$\endgroup\$ Commented Aug 15, 2015 at 1:03
  • \$\begingroup\$ Something like delete word from wordlist \$\endgroup\$ Commented Aug 15, 2015 at 7:19
  • \$\begingroup\$ Can you give us more information about the class on which the delete method will be added? Is this part of a module that gets included in other classes? \$\endgroup\$ Commented Aug 17, 2015 at 13:32

2 Answers 2

2
\$\begingroup\$

Some notes:

  • An array is not the best data structure to store the words, operations will be O(n). Better a hash {word_id => word}

  • Use 2-space indentation.

Then you can write:

define_singleton_method(:delete) do |id|
  if @@words.delete(id)        
    Definition.delete_by_word_id(id)
  end
end
\$\endgroup\$
1
\$\begingroup\$

The method would benefit from functional programming:

@@words.select{|word| word.id == id}.each{|id| Definition.delete_word_by_id(id)}
@@words.select{|word| word.id != id}

Still I don't like that this method both has side effects and returns, it has too much responsability.

\$\endgroup\$

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.