Quick and dirty command line application in Ruby. Now it's time to refactor, where should I start?

Question

#!/usr/bin/env ruby
require 'pivotal-tracker'
require 'yaml'
TEMP_DIR = "#{ENV['HOME']}/.pivotal/" 
TEMP_FILE = TEMP_DIR + "temp.yml"
CONFIG_FILE = TEMP_DIR + "config.yml"
unless File.directory? TEMP_DIR
 Dir::mkdir TEMP_DIR
end
unless File.file? TEMP_FILE
 f = File.open(TEMP_FILE, 'w')
 f.write("id: -1")
end
unless File.file? CONFIG_FILE
 f = File.open(CONFIG_FILE, 'w')
 f.write("")
end
# TEMP FILE format:
 # current_story_id: 123 OR -1
def story_info story
 puts story.name
 puts "id:\t\t#{story.id}"
 puts "notes:\t\t#{story.description}"
 puts "status:\t\t#{story.current_state}"
 puts "estimate:\t#{(story.estimate == -1) ? 'unestimated' : story.estimate}"
end
def is_next string
 string == 'next'
end
def id string
 !string.match(/^[\d]+(\.[\d]+){0,1}$/).nil?
end
def current string
 string == 'current'
end
def story_has_been_started
 f = YAML.load_file(TEMP_FILE)
 @current_id = f['id']
 f['id'] != -1
end
def update_id id
 f = File.open(TEMP_FILE, 'w')
 f.write("id: #{id}")
 @current_id = id
end
def set_config key, value
 config = YAML::load(File.read(CONFIG_FILE)) || {}
 config[key.to_sym] = value
 open(CONFIG_FILE, 'w') { |f| YAML::dump(config, f) }
end
def authenticate
 config = YAML::load(File.read(CONFIG_FILE)) || {}
 has_email = true, has_password = true
 unless config.has_key? :email
 puts "033円[33mUse pivotal set email EMAIL to set your pivotal tracker email033円[0m\n"
 has_email = false
 end
 unless config.has_key? :password
 puts "033円[33mUse pivotal set password PASSWORD to set your pivotal tracker email033円[0m\n"
 has_password = false
 end
 unless has_email and has_password
 exit 0
 end
 PivotalTracker::Client.token(config[:email], config[:password])
end
@current_id = nil
begin
 unless ARGV[0] == 'set'
 authenticate
 aidin = PivotalTracker::Project.all.first
 end
rescue
 puts "033円[33mInvalid Credentials033円[0m\n"
 exit 0
end
case ARGV[0]
when "info"
 # pivotal info next
 # pivotal info id
 # pivotal info current
 if is_next(ARGV[1])
 puts "033円[32mDisplaying information for next story033円[0m\n"
 story_info(aidin.stories.all(owner: 'Chintan Parikh', state: 'unstarted').first)
 elsif id(ARGV[1])
 story = aidin.stories.find(Integer(ARGV[1]))
 unless story.nil?
 puts "033円[32mDisplaying information for story #{ARGV[1]}033円[0m\n"
 story_info(story)
 else
 puts "033円[33mNo story with id #{ARGV[1]} exists033円[0m\n"
 end
 elsif current(ARGV[1])
 if story_has_been_started
 story = aidin.stories.find(@current_id)
 puts "033円[32mDisplaying information for current story033円[0m\n"
 story_info(story)
 else 
 puts "033円[33mNo story has been started. Use pivotal start to start a story first033円[0m\n"
 end
 end
when "estimate"
 # pivotal estimate next 1-8
 # pivotal estimate id 1-8
 # pivotal estimate current 1-8
when "start"
 # pivotal start next
 # pivotal start id
 if story_has_been_started
 current_branch = `git branch | grep "*" | sed "s/* //"`.chomp
 status = `git status -s`
 if !status.empty?
 puts "033円[33mYou are currently working on story #{@current_id} and have uncommitted changes on #{current_branch}. If you continue, your uncommitted changes will be lost. Continue? (Y/N)033円[0m\n"
 continue = false
 while (!continue)
 option = $stdin.gets.chomp
 if option == 'N'
 exit 1
 elsif option != 'Y'
 puts "Please enter either Y or N"
 else
 continue = true;
 end
 end
 end
 end
 `git stash`
 `git stash drop`
 `git checkout develop`
 `git pull`
 if is_next(ARGV[1])
 story = aidin.stories.all(owner: 'Chintan Parikh', state: 'unstarted').first
 new_branch = "feature/#{story.id}_#{story.name.downcase.gsub(' ', '_').gsub(/[^0-9A-Za-z_]/, '')}"
 `git checkout -b #{new_branch}`
 elsif id(ARGV[1])
 story = aidin.stories.find(Integer(ARGV[1]))
 unless story.nil?
 new_branch = "feature/#{story.id}_#{story.name.downcase.gsub(' ', '_').gsub(/[^0-9A-Za-z_]/, '')}"
 `git checkout -b #{new_branch}`
 else
 puts "033円[33mNo story with id #{ARGV[1]} exists033円[0m\n"
 end
 end
 update_id(story.id) unless story.nil?
 story.update(current_state: 'started')
 puts "033円[32mStory #{story.id} has been started033円[0m\n"
 story_info(story)
when "complete"
 if story_has_been_started
 story = aidin.stories.find(@current_id)
 branch = "feature/#{story.id}_#{story.name.downcase.gsub(' ', '_').gsub(/[^0-9A-Za-z_]/, '')}"
 `git push origin #{branch}`
 story.update(current_state: 'finished')
 story.update(current_state: 'delivered')
 puts "033円[32mStory #{story.id} has been completed033円[0m\n"
 update_id(-1)
 else
 puts "033円[33mNo story has been started. Use pivotal start to start a story first033円[0m\n"
 end
when 'abandon'
 if story_has_been_started
 story = aidin.stories.find(@current_id)
 story.update(current_state: 'unstarted')
 puts "033円[32mStory #{story.id} has been abandoned033円[0m\n"
 update_id(-1)
 else
 puts "033円[33mNo story has been started. Use pivotal start to start a story first033円[0m\n"
 end
when 'list'
 stories = aidin.stories.all(owner: 'Chintan Parikh', current_state: ['unstarted', 'started', 'finished', 'delivered'])
 stories.each do |story|
 story_info(story)
 end
when 'set'
 # set email
 # set password
 set_config(ARGV[1], ARGV[2])
 puts "033円[32m#{ARGV[1]} has been set to #{ARGV[2]}033円[0m\n"
end

It's a pretty basic application to simplify my workflow where I'm interning. I'd like to chuck it on my github, but I'd rather the code was a little nicer before I throw it up for the world to see. Any idea where, or how I should start refactoring?

• Flambino: wants code to be "nicer before I throw it up for the world to see" - posts code on the internet anyway :) But in all seriousness, I'd probably start by removing the hard coded name. I'm thinking it should be possible to get your name via the pivotal library you're using, since you already give it an email and password (and that config might work better as environment variables or a ~/.pivotalconfig - or something - file)
• Mohammad: You should really try rubocop, it is fascinating!

Answer 1

I haven't used Pivotal Tracker, so I can't really say what's "right" to do, purpose-wise. However, skimming through your code, I have a few ideas:

1. Extend PivotalTracker's classes for fun and profit

Instead of a global method like story_info (which I'd prefer was called print_story_info as that what it does), I'd probably extend the PivotalTracker::Story class with an info, inspect, or to_s method, so you can simply write puts story.info when necessary. Something like

module PivotalTracker
 class Story
 def info
 <<-EOT
 #{name}
 ID: #{id}
 Notes: #{description}
 Status: #{story.current_state}
 Estimate: #{estimate == -1 ? 'none' : estimate}
 EOT
 end
 end
end

Do the same to add a class method for finding "your" stories, and whatever else you need to do with Pivotal.

2. Those string-checking functions...

Your is_next, id and current methods smell pretty grim. For one, it should - if anything - be next?, id?, and current?, but really there's no need for them to begin with. You only use them twice in your code. Yes, yes, DRY and all that, but honestly, a simple string == "next" comparison does not need to be abstracted into a method.

Now, if you were doing something more fault-tolerant like downcasing before comparing, or allowing short-hand args like "n" for "next", then maybe. But even so, the methods would still smell. Remember, in Ruby you can use regular expressions directly in string switches, e.g.

case some_string
when /^n(ext)?$/i
 # do stuff
case...

That said, your code could absolutely benefit from encapsulating its logic in more methods and classes, but this isn't the place to start.

I'd recommend using an option/command line switch parser to do the ARGV-checking/-translation heavy-lifting, and in general restructuring stuff. Right now, your script is basically bash written in Ruby, with big, global switches and all. But it could be a lot cleaner with more OOP and a few helpful gems.

3. Hand-coded terminal styling

Hand-coding the styles makes the code harder to read and more error-prone (cosmetic errors, but still). Use a coloring gem of some sort to do that stuff for you (and to check if the terminal even supports colors and styles!)

4. Check that shell commands actually succeed

In your start, er, "function", you perform a lot of git commands, some very destructive. I'd do some serious checking before and after that. In particular I'd at least check $?.success? a few times to make sure that a command actually succeeded before running the next one.

5. The estimate argument

All commented out → just remove it.
Just saying :)

Also see the comment I posted to your question (i.e. don't hardcode your name in there)