current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 4 down vote favorite
1

I'm a Java developer, I'm new to Ruby and I'm worried that I'm forcing or not-so-well using the goodness of the Ruby syntax.

  • What do you think about the Exception catching and how to print it in error messages?
  • What about the code block (closure?) I'm passing to the "until" method?
  • Is it ok to implicitly rely on what the last executed sentence of the block will be, regarding what would be its returned value?
def waitUntilDisappears(type, name) #Waits for a particular element to disappear
  begin
    puts "Waiting for element #{name} to disappear..."
    wait = Selenium::WebDriver::Wait.new(:timeout => 5)
    wait.until do
      element = driver.find_element(type, name)
      if element != nil
        displayValue = element.css_value("display")
        puts "Element #{name} has displayValue #{displayValue}."
        displayValue != "block"
      end
    end
    puts "Element #{name} disappeared or not present. OK."
  rescue Exception => e
    puts "Error: Could not wait for element #{name} to disappearDetails: #{e.inspect}"
  end
end
share|improve this question
    
Thanks for the edit, I added "Ruby" to the title because Selenium here is used via Ruby (it could be other languages like Java, etc, but this case is the use of Ruby i'm interested in reviewing). – jotadepicas Jan 13 '15 at 19:03
up vote 2 down vote accepted

Some notes:

  • waitUntilDisappears. In Ruby, always: wait_until_disappears.
  • if element != nil -> if element.
  • if element != nil. I think find_element never returns nil, it raises exception if not found, so this is not needed.
  • rescue Exception => e. A rescue for all the method can omit the begin.
  • rescue Exception => e. Rescuing from Exception is bad practice. Rescue StandardError, which is the same as: rescue => e.
  • puts "Error. It's bad practice to catch an exception (all the worse if it's all exceptions), just print to the screen and happily return as if nothing happened. Raise an exception or return a value that signals the error.
  • Is Element#visible? not enough?

I'd write:

def wait_until_disappears(type, name, timeout: 5)
  wait = Selenium::WebDriver::Wait.new(timeout: timeout)
  wait.until { !driver.find_element(type, name).visible? }
end
share|improve this answer
    
Hey, thanks! Regarding Element#visible it works different on different browsers, I've seen that most people check manually for the "display" attribute. About the "catch & log" of the exceptions, I'm currently refactoring this testing library, It would probably become a "catch, log and throw", so I can add some context information to the error. Thanks again! – jotadepicas Jan 15 '15 at 14:07

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.