current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite

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
share|improve this question
    
Please provide a more specific title. "Delete method" sounds too generic. –  Jamal♦ Aug 15 at 1:03
    
Something like delete word from wordlist –  Caridorc Aug 15 at 7:19
    
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? –  Greg Burghardt Aug 17 at 13:32

2 Answers 2

up vote 1 down vote

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.

share|improve this answer
up vote 2 down vote

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
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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