1
\$\begingroup\$

I come from a C background and need help refactoring the following imgur image uploader script I wrote.

It checks if a token_file with access_token and refresh_token exists. If not, it instructs the user to go to a webpage, allow access to my app and enter the PIN generated from the site back to my program so that I can exchange it with access and refresh tokens.

I upload the picture taken with the 'scrot' utility to the users album, put the direct link on clipboard, and show a system notification. I'm not using the libnotify gem because it segfaults.

If the upload failed because the access_token is expired, which lasts only 60 minutes, I use the refresh token I got from the first authorization step, get a new one and update the token_file as well.

I know I should be using a class to keep things more organized.

I just don't know what's the best way to deal with info like client_id and token_file. Should they be instance or class methods? Also what should I put in the initialize method, the reading of tokens from the file or launch scrot?

I know I don't error check for everything yet.

#!/usr/bin/env ruby

require 'httparty'
require 'json'
require 'clipboard'

$client_id     = 'xxxxxxxxxxx'
$client_secret = 'xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx'
$token_file    = '.imgur_token'


def auth_app

  puts 'Follow the link to allow the application access to your account and enter the pin',
  "https://api.imgur.com/oauth2/authorize?client_id=#{$client_id}&response_type=pin"

  print 'Pin: '
  pin = STDIN.gets.chomp

  response = HTTParty.post 'https://api.imgur.com/oauth2/token',
    :body => { 'client_id'     => $client_id,
               'client_secret' => $client_secret,
               'grant_type'    => 'pin',
               'pin'           => pin }

  if response['access_token'] == nil
    puts 'Authorization failed'
  else
    tokens = { 'access_token'  => response['access_token'],
               'refresh_token' => response['refresh_token'] }
    File.write($token_file, tokens.to_json)
  end
  tokens
end


def refresh_token(refresh_token)

  response = HTTParty.post 'https://api.imgur.com/oauth2/token',
    :body => { 'refresh_token' => refresh_token,
               'client_id'     => $client_id,
               'client_secret' => $client_secret,
               'grant_type'    => 'refresh_token' }

  response['access_token']
end


def upload_image(access_token)

  response = HTTParty.post 'https://api.imgur.com/3/upload.json',
    :headers => { 'Authorization' => "Bearer #{access_token}" },
    :body    => { 'image' => Base64.encode64(File.read('imgur.png')) }

  response['data']['link']
end


abort('scrot not found') unless system('scrot -s imgur.png')
tokens = File.exists?($token_file) ? JSON.parse(File.read($token_file)) : auth_app

if (link = upload_image(tokens['access_token'])) == nil
  tokens['access_token'] = refresh_token(tokens['refresh_token'])
  link = upload_image(tokens['access_token'])
  File.write($token_file, tokens.to_json)
end

if link != nil
  Clipboard.copy link
  system('notify-send Upload complete')
else
  system('notify-send Upload error')
end

File.delete('imgur.png')
\$\endgroup\$
  • \$\begingroup\$ As a suggestion, using $ global variables is a first sign something is awry. CONSTANTS are a better choice usually, or pass the value in as a parameter to the method. \$\endgroup\$ – the Tin Man Jul 2 '13 at 18:23
1
\$\begingroup\$

I'd do it something like this. Comments are sprinkled throughout:

#!/usr/bin/env ruby

require 'httparty'
require 'json'
require 'clipboard'

Use constants instead of globals:

CLIENT_ID     = 'xxxxxxxxxxx'
CLIENT_SECRET = 'xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx'
TOKEN_FILE    = '.imgur_token'

Move strings that are subject to change to the top of the file for easy access:

AUTHORIZE_URL = "https://api.imgur.com/oauth2/authorize?client_id=#{ CLIENT_ID }&response_type=pin"
TOKEN_URL = 'https://api.imgur.com/oauth2/token'

def auth_app

I prefer to see separate strings being printed on individual lines, rather than split them with , and let puts do it. It helps the code reflect what the output will be:

  puts 'Follow the link to allow the application access to your account and enter the pin:'
  puts AUTHORIZE_URL

  print 'Pin: '
  pin = STDIN.gets.chomp

I know it's common/popular in Ruby to not use parenthesis to delimit parameters to methods, but I always use them. It visually separates the parameters from the method name, and also forces the correct parsing in ambiguous cases when blocks are also passed in.

Also, I prefer to see the parameters on separate lines from opening/closing braces and brackets when there is more than a trivial array or hash. Again, it's a readability thing:

  response = HTTParty.post(
    TOKEN_URL,
    :body => {
      'client_id'     => CLIENT_ID,
      'client_secret' => CLIENT_SECRET,
      'grant_type'    => 'pin',
      'pin'           => pin 
    }
  )

I reversed the order of the test for clarity and simplicity. Rather than test for nil, I test for the "positive" condition, which is a simpler test:

  if response['access_token']
    tokens = {
      'access_token'  => response['access_token'],
      'refresh_token' => response['refresh_token'] 
    }
    File.write(TOKEN_FILE, tokens.to_json)
  else
    puts 'Authorization failed'
  end
  tokens
end

def refresh_token(refresh_token)

  response = HTTParty.post(
    TOKEN_URL,
    :body => {
      'refresh_token' => refresh_token,
      'client_id'     => CLIENT_ID,
      'client_secret' => CLIENT_SECRET,
      'grant_type'    => 'refresh_token' 
    }
  )

  response['access_token']
end

def upload_image(access_token)

  response = HTTParty.post 'https://api.imgur.com/3/upload.json',
  :headers => { 'Authorization' => "Bearer #{access_token}" },
  :body    => { 'image' => Base64.encode64(File.read('imgur.png')) }

  response['data']['link']
end

abort('scrot not found') unless system('scrot -s imgur.png')

if File.exists?(TOKEN_FILE)
  tokens = JSON.parse(File.read(TOKEN_FILE))
else
  tokens = auth_app
end

I do NOT like seeing an assignment inside the conditional test. It's confusing and an error-in-waiting in the future if someone things you meant to do an equality check and "corrects" the code:

link = upload_image(tokens['access_token'])

Again, I reversed the test:

if (!link)
  tokens['access_token'] = refresh_token(tokens['refresh_token'])
  link = upload_image(tokens['access_token'])
  File.write(TOKEN_FILE, tokens.to_json)
end

I reversed the test and assign a message to a string so I can fall through and use a single system invocation:

if link
  response_msg = 'notify-send Upload error'
else
  Clipboard.copy link
  response_msg = 'notify-send Upload complete'
end
system(response_msg)

File.delete('imgur.png')

In general I think your code is pretty good, with just a few tweaks that I'd do. I'd rather write my code as clearly and cleanly as I can, even for trivial stuff, because I never know when I'll need to revisit it. Sometimes I have to tweak something a year or two later, so being able to comprehend what the heck I was thinking quickly is important.

|improve this answer|||||
\$\endgroup\$

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.