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 3 down vote favorite
1

I have a class Creator which will execute a block code for a number of times. I'm not sure how to write this in a more elegant way in Ruby.

class Creator
    attr_accessor :block
    def self.create(&block)
        @block = block
        return self
    end

    def self.for(number)
        0.upto(number) {
            block.call
        }
    end
end

And this is how I call it which I want to stay like this.

Creator.create{ 
  SomeModel.create!(@attr)
}.for(30)
share|improve this question
1  
I am sure you have your reasons, but what about 30.times { SomeModel.create!(@attr) }. –  tokland Jan 14 '14 at 11:16
    
Yeah, that's one option that I was looking at as well. But I'm trying to learn code/block in ruby and I'm planning to do some more in this class as well. –  toy Jan 14 '14 at 11:25

3 Answers 3

up vote 3 down vote accepted

You want instance methods, not class methods. With the current code, you would get:

hello = Creator.create { puts "Hello" }
goodbye = Creator.create { puts "Goodbye" }
hello.for(1)   # Prints "Goodbye"

Assuming you don't want block to be changed afterwards, I would change attr_accessor to attr_reader. I don't see much reason to even expose a reader, though — it seems that doing so would only lead to mischief.

class Creator
  # For compatibility with the old API
  def self.create(&block)
    return self.new(&block)
  end

  def initialize(&block)
    @block = block
  end

  def for(number)
    number.times { @block.call }
  end
end

This is just a syntactic sugar wrapper for blocks. Perhaps a more generic name than Creator might be appropriate. It might also be simpler to call number.times { block.call } directly.

Standard indentation for Ruby is two spaces.

share|improve this answer
up vote 2 down vote

This can be done simply enough inline that I don't think the creator class is worth the trouble:

30.times.map { SomeModel.create!(@attr) }

30.times creates an Enumerator that yields 30 times, then map turns each yield into a new instance of SomeModel.

or, if you don't need the actual instances:

30.times { SomeModel.create!(@attr) }
share|improve this answer
1  
apparently the OP wants the code to work by side-effects (not a great idea) and the map is not necessary. –  tokland Jan 14 '14 at 11:20
    
@tokland, Thanks. Edited. When dealing with the database, it's all about side-effects. What the OP wants done doesn't seem bad to me (although the heavyweight machinery to do it does). –  Wayne Conrad Jan 14 '14 at 13:24
up vote 1 down vote

With attr_accessor :block you define a accessor for an instance.

You may use the variable @block directly:

class Creator
    def self.create(&block)
        @block = block
        return self
    end

    def self.for(number)
        0.upto(number) {
            @block.call
        }
    end
end


Creator.create{ 
  p 'here' #your SomeModel.create!(@attr)
}.for(30)

Or if you think you need an accessor:

class Creator
  class << self
    attr_accessor :block
    def create(&block)
        self.block = block
        return self
    end

    def for(number)
        0.upto(number) {
            block.call
        }
    end
  end
end


Creator.create{ 
  p 'here' #your SomeModel.create!(@attr)
}.for(30)

But there is no need of for and create. I would define only one method that accepts the number of executions:

class Creator
    def self.create(number, &block)
        number.times{
            block.call
        }
    end
end

Creator.create(30){ 
  p 'here' #your SomeModel.create!(@attr)
}
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.