How should I refactor my image uploader script?

Question

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

• the Tin Man: 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.

Answer 1

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.