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}&amp;response_type=pin"

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

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

  if response['access_token'] == nil
    puts 'Authorization failed'
  else
    tokens = { 'access_token'  =&gt; response['access_token'],
               'refresh_token' =&gt; 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 =&gt; { 'refresh_token' =&gt; refresh_token,
               'client_id'     =&gt; $client_id,
               'client_secret' =&gt; $client_secret,
               'grant_type'    =&gt; 'refresh_token' }

  response['access_token']
end


def upload_image(access_token)

  response = HTTParty.post 'https://api.imgur.com/3/upload.json',
    :headers =&gt; { 'Authorization' =&gt; "Bearer #{access_token}" },
    :body    =&gt; { 'image' =&gt; 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')
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 }&amp;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 =&gt; {
      'client_id'     =&gt; CLIENT_ID,
      'client_secret' =&gt; CLIENT_SECRET,
      'grant_type'    =&gt; 'pin',
      'pin'           =&gt; 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'  =&gt; response['access_token'],
      'refresh_token' =&gt; 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 =&gt; {
      'refresh_token' =&gt; refresh_token,
      'client_id'     =&gt; CLIENT_ID,
      'client_secret' =&gt; CLIENT_SECRET,
      'grant_type'    =&gt; 'refresh_token' 
    }
  )

  response['access_token']
end

def upload_image(access_token)

  response = HTTParty.post 'https://api.imgur.com/3/upload.json',
  :headers =&gt; { 'Authorization' =&gt; "Bearer #{access_token}" },
  :body    =&gt; { 'image' =&gt; 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.