60
\$\begingroup\$

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.)

\$\endgroup\$
4
  • 8
    \$\begingroup\$ Have an upvote: you'll be wanting to use the chatroom! \$\endgroup\$ Commented Mar 16, 2014 at 15:03
  • 7
    \$\begingroup\$ I can't believe I'm talking to a robot. This is awesome! \$\endgroup\$ Commented Mar 16, 2014 at 19:29
  • 9
    \$\begingroup\$ Ahem, I believe I licensed that under MIT, meaning you have to give attribution to me. ;) I'd prefer "The Supreme Overlordly Knob of the Door, Superior to Mankind in All Ways," but anything goes \$\endgroup\$ Commented Apr 23, 2014 at 13:14
  • \$\begingroup\$ Related: Chat bot feature requests on meta. \$\endgroup\$ Commented Sep 20, 2015 at 19:40

4 Answers 4

Reset to default
22
\$\begingroup\$

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." }
\$\endgroup\$
1
  • \$\begingroup\$ Thanks for the review! In response to your comment in the code, take a look at the SE API here. \$\endgroup\$ Commented Mar 16, 2014 at 17:03
22
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks for the review! I guess I was too used to my C-syntactical ways. :) \$\endgroup\$ Commented Mar 16, 2014 at 21:25
  • \$\begingroup\$ @syb0rg It seems to be accepted, in C, to write long methods. I've never understood the practice. \$\endgroup\$ Commented Mar 16, 2014 at 21:28
8
\$\begingroup\$

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.
\$\endgroup\$
1
  • 1
    \$\begingroup\$ There is, I agree, pretty good consensus that parentheses should not be omitted in method declarations. What about method calls? \$\endgroup\$ Commented Mar 16, 2014 at 19:01
3
\$\begingroup\$

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}
\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.