4
\$\begingroup\$

I've written a module for repeating blocks of code, generally to cover issues related to eventual consistency and testing screen elements which may take some time to fully load. It looks like this:

class Repeater
  def initialize(&block)
    @repeat_block = block
  end

  def repeat(times: 25, delay: 0.2)
    result = nil
    times.times do
      result = @repeat_block.call
      break if @until_block.present? && @until_block.call
      sleep(delay)
    end
    result
  end

  def until(&block)
    @until_block = block
    self
  end
end

The Repeater takes two blocks of code, one to be run and the second to check for an exit criteria. Optionally the number of repeats and delays can be overwritten after the .repeat method.

This is implemented in many places throughout the codebase, an example of such is:

  result = false
  Repeater.new do
    result = event_present_in_database
  end.until do
    result
  end.repeat(times: 10, delay: 0.1)
  result

In this case the event_present_in_database method is being repeated called until it returns true instead of false, which indicates that the event has been created as expected, or else times out indicating there's an issue.

This certainly works for what I want it to do, however it doesn't seem very neat and I'm sure it could be tidied up. I know Ruby has all sorts of tricks that I've yet to come across, is there a way to leverage these to make this nicer to use/easier to read.

\$\endgroup\$
  • \$\begingroup\$ This isn't your actual code, is it? Code Review doesn't handle hypothetical code well, please upload an actual implementation so it can be meaningfully reviewed. \$\endgroup\$ – Mast Oct 10 '19 at 9:44
  • 1
    \$\begingroup\$ The repeater I want to refactor is real world code, the implementation was generic as it's being used multiple times throughout the code. Have replaced the example with an actual usage if that clears things up \$\endgroup\$ – Psycrow Oct 10 '19 at 10:05
  • \$\begingroup\$ Thank you, that does help. \$\endgroup\$ – Mast Oct 10 '19 at 12:11
  • \$\begingroup\$ This could become a good question, because I do find this code to be awkward. However, the example use case is too simplified and hypothetical, in my opinion. It's unclear what you would want to do with result, and how event_present_in_database goes about doing its work. There could be other polling/retrying mechanisms you could use, such as try-catch, but I can't give you good advice based on this sketchy code. \$\endgroup\$ – 200_success Oct 10 '19 at 18:37
  • 2
    \$\begingroup\$ The Repeater class is being used in multiple different scenarios, the given code is just one such example which calls a method repeatedly until either it returns true or the maximum number of repeats is met, it is used to test that events are eventually being made even though there may be a small delay. Elsewhere in the project I'm using the repeater to check for an on-screen element to be present, but which can sometimes take a moment to load. As such the actual method being called isn't really relevant as it can be anything that returns a value, however I'll expand on how it currently works. \$\endgroup\$ – Psycrow Oct 10 '19 at 23:41
3
\$\begingroup\$

There's not much weak points I can see.

The biggest one is the way to use it. One needs to declare a variable to be caught in the block (closure), and I think it's unnecessary. You should be able to do this:

  Repeater.new do
    event_present_in_database
  end.until do |result|
    result
  end.repeat(times: 10, delay: 0.1)

If you changed the break like like this:

  break if @until_block.present? && @until_block.call(result)

The second, tiny point would be: make it clearer to understand the extraction to method conditionals:

  # ...
  break if exit_criteria_satisfied?(result)
  # ...

  private 
  def exit_criteria_satisfied?(result)
    @until_block.present? && @until_block.call
  end

That's all I got. The rest of the code seems pretty clear to me.

I know Ruby has all sorts of tricks

Yeah, but I don't know a single one that could help with the readability here.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ This is interesting, and is the sort of thing that I had in mind. One question, is there a way to do this if I need to interrogate the result of the Repeater? For example, if I'm waiting until there is a record present in the database, but once it's there I want to use the values within the record to do something else. My first thought is to store result as an instance variable, any better ways to do it? \$\endgroup\$ – Psycrow Nov 19 '19 at 11:28
  • \$\begingroup\$ Actually was just being a bit dense, you can assign the output of Repeater to a variable to have it accessible once it's finished running. Am going to experiment with this a bit and see how it handles real world applications \$\endgroup\$ – Psycrow Nov 19 '19 at 14:24
  • \$\begingroup\$ Yeah, repeat method still returns the last result from the block, so you can assign it to any variable. \$\endgroup\$ – Grzegorz Nov 19 '19 at 14:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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