How should I refactor my image uploader script?

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

else
end

File.delete('imgur.png')

• 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. – the Tin Man Jul 2 '13 at 18:23

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

#!/usr/bin/env ruby

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


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

:headers => { 'Authorization' => "Bearer #{access_token}" },
:body    => { 'image' => Base64.encode64(File.read('imgur.png')) }

end

if File.exists?(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'])
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