Chat bot for posting recent answers

Question

I am the half-robot side of syb0rg that will be posting the recent answers of Code Review to the CR Answers chatroom. Here is the list of review suggestions I would like, in order of preference:

1. Efficiency (with API requests, speed of login and posting answers, etc.)
2. Security issues
3. Best practices

For feature requests regarding the chat bot, please see this meta post.

Any and all reviews are acceptable however. Don't be too harsh please, this is one of my first times using Ruby.

ACCESS_TOKEN = '<insert key>'
# get your access token here:
# https://stackexchange.com/oauth/dialog?client_id=2666&redirect_uri=http://keyboardfire.com/chatdump.html&scope=no_expiry
$root = 'http://stackexchange.com'
$chatroot = 'http://chat.stackexchange.com'
$room_number = 12723
site = 'codereview'
email = '<insert email>'
password = '<insert password>'

require 'rubygems'
require 'mechanize'
require 'json'
require 'net/http'

loop
{
    begin

    $agent = Mechanize.new
    $agent.agent.http.verify_mode = OpenSSL::SSL::VERIFY_NONE

    login_form = $agent.get('https://openid.stackexchange.com/account/login').forms.first
    login_form.email = email
    login_form.password = password
    $agent.submit login_form, login_form.buttons.first
    puts 'logged in with SE openid'

    meta_login_form = $agent.get($root + '/users/login').forms.last
    meta_login_form.openid_identifier = 'https://openid.stackexchange.com/'
    $agent.submit meta_login_form, meta_login_form.buttons.last
    puts 'logged in to root'

    chat_login_form = $agent.get('http://stackexchange.com/users/chat-login').forms.last
    $agent.submit chat_login_form, chat_login_form.buttons.last
    puts 'logged in to chat'

    $fkey = $agent.get($chatroot + '/chats/join/favorite').forms.last.fkey
    puts 'found fkey'

    def send_message text
        loop 
        {
            begin
            resp = $agent.post("#{$chatroot}/chats/#{$room_number}/messages/new", [['text', text], ['fkey', $fkey]]).body
            success = JSON.parse(resp)['id'] != nil
            return if success
            rescue Mechanize::ResponseCodeError => e
            puts "Error: #{e.inspect}"
            end
            puts 'sleeping'
            sleep 3
        }
    end

    puts $ERR ? "An unknown error occurred. Bot restarted." : "Bot initialized."

    last_date = 0
    loop 
    {
        uri = URI.parse "https://api.stackexchange.com/2.2/events?pagesize=100&since=#{last_date}&site=#{site}&filter=!9WgJfejF6&key=thqRkHjZhayoReI9ARAODA((&access_token=#{ACCESS_TOKEN}"
        http = Net::HTTP.new(uri.host, uri.port)
        http.use_ssl = true
        http.verify_mode = OpenSSL::SSL::VERIFY_NONE
        data = JSON.parse http.get(uri.request_uri).body
        events = data['items']

        data['items'].each do |event|
            last_date = [last_date, event['creation_date'].to_i + 1].max
            if ['answer_posted'].include? event['event_type']
                send_message "[tag:rob0t] New answer detected:"
                send_message event['link']
                puts "Answer posted."
            end
        end
        puts "#{data['quota_remaining']}/#{data['quota_max']} quota remaining"
        sleep(40 + (data['backoff'] || 0).to_i) # add backoff time if any, just in case
    }

    rescue => e
        $ERR = e
        p e
    end
}

_{(Attribution to original author, code above is a modified version of it.)}

Answer 1

For starters, indent your code consistently — the standard in Ruby is two spaces. That includes indenting the contents of your begin-rescue-end blocks.

Normally, I don't like to make such a huge fuss about indentation, but in this case I think it's highly important, because:

1. Your program has a highly unusual outline (infinite loops and a function definition(!) inside an infinite loop)
2. The stakes are high: if you misbehave, you could make a lot of people upset. Therefore, good software engineering practices should be used.

An outline like this would be more idiomatic for Ruby:

class AnswerBot
  ROOT = 'http://stackexchange.com'
  CHAT_ROOT = 'http://chat.stackexchange.com'

  def initialize(options)
    @agent = Mechanize.new
    @options = options
  end

  def login
    # Do stuff with @agent
    login_form = $agent.get('https://openid.stackexchange.com/account/login').forms.first
    login_form.email = @options[:email]
    # ...
    @fkey = @agent.get(CHAT_ROOT + '/chats/join/favorite').forms.last.fkey
  end

  def fetch_answers
    # Make request to api.stackexchange.com
    # ...
    data['items'].each { |event| yield event }
    return (data['backoff'] || 0).to_i
  end

  def send_message(text, retries=5, backoff=40)
    # ...
  end
end

bot = AnswerBot.new(:access_token => ...,
                    :room_number = 12723,
                    :site => 'codereview',
                    :email => ...,
                    :password => ...)
loop {
  begin
    bot.login

    do
      backoff = bot.fetch_answers do |event|
        if ['answer_posted'].include?(event['event_type']) # <-- Is that right?
          bot.send_message(...)
        end
      end
    while sleep(40 + backoff)
  rescue => e
    puts "An error occurred."
    p e
  end
  puts "Bot restarted."
}

Answer 2

Block syntax

This:

loop
{
  ...
}

Causes a syntax error in MRI 2.1. This would fix the syntax error:

loop {
  ...
}

However, the use of {...} is normally reserved for single-line blocks. Prefer:

loop do
  ..
end

Methods

Use many more methods. It should be possible to figure out what the script does, in broad strokes, by looking only at its main method. Find lines of code that do one thing and put them in their own method. For example:

def login_to_se
  login_form = $agent.get('https://openid.stackexchange.com/account/login').forms.first
  login_form.email = email
  login_form.password = password
  $agent.submit login_form, login_form.buttons.first
  puts 'logged in with SE openid'
end

...

login_to_se

and so on. Your methods should, when possible, have these properties:

• The method does one thing
• The name says what it does
• All of the code in the method is at the same level of abstraction

You want code, at the higher levels such as the main loop, to look more like this:

loop do
  continue_on_error do
    login_to_se
    login_to_meta
    login_to_chat
    loop do
      copy_new_post_to_chat
      wait
    end
  end
end

A method should read like a story. Abstract away--in methods, classes, etc--details that make the story hard to follow.

Abstract out rescue, too

You may notice the call to continue_on_error above. It can be very useful to abstract out your rescue blocks, too. In this case, it gives us a method name that documents why we are doing the rescue:

def continue_on_error
  yield
rescue => e
  $ERR = e
  p e
end

$ERR

We can get rid of $ERR by having #continue_on_error say that we're restarting:

def continue_on_error
  yield
rescue => e
  puts e
  puts "Restarting"
end

and in the main loop, instead of:

puts $ERR ? "An unknown error occurred. Bot restarted." : "Bot initialized."

simply

puts "Initialized"

The script's log output will be just as clear.

Answer 3

Some low-level style issues:

• Although parentheses around parameter lists are optional, there is consensus that they should not be omitted.
• I don't see any consist pattern in your use the $ sigil for variables. I suggest not using them at all.
• You use both Mechanize and raw Net::HTTP requests. I suggest using Mechanize for everything.

Answer 4

Adding to the existing answers:

1. You don't need to require rubygems as you are not using it at all. It is usually unnecessary. See here https://stackoverflow.com/questions/2711779/require-rubygems .

2. When you have many requires you can do this trick to group them into one line:

   require 'rubygems'
   require 'mechanize'
   require 'json'
   require 'net/http'
   

   Into

   %w{rubygems mechanize json net/http}.each{|gem| require gem}