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\$
    – Jamal
    Commented Aug 15, 2015 at 1:03
  • \$\begingroup\$ Something like delete word from wordlist \$\endgroup\$
    – Caridorc
    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\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.