Problem
I made this application that runs fine, but I feel the code is sloppy as there are many if statements. I’m a beginner so I’m not sure how to improve it, at least not without messing everything up. Any suggestions?
#!/usr/bin/ruby
require 'thor'
require 'json'
class ListTrends < Thor
desc "list_trends json", "list out trends from JSON file"
option :api_key, :aliases => "--api-key", :type => :string, :required => true
option :format, :type => :string, :desc => "one line format"
option :no_country_code, :aliases => "--no-country-code", :desc => "remove country code", :type => :boolean
def list_trends(keyword=nil)
json = File.read('trends_available.json')
trends_hash = JSON.parse(json)
re = /^(?=.*[a-zA-Z])(?=.*[0-9]).{8,}$/
keyword = keyword.to_s.downcase
if re.match(options[:api_key])
trends_hash.each do |trend|
if trend["country"].downcase.include?(keyword)
if options[:format]
output = trend.values[0..2]
output.delete_at(1) if options[:no_country_code]
puts output.join(" ")
else
# Complete output
trend.each do |k, v|
if v.is_a?(Hash)
v.each do |i, j| puts "Trend location type #{i}: #{j}" end
else
puts "Trend #{k}: #{v}"
end
end # trend.each do
puts ""
end # if options[:format]
end # if trend["country"]
end # trends_hash.each
else
puts "Invalid API Key, operation abort..."
end # if re.match
end # list_trends
end
ListTrends.start(ARGV)
Solution
The way to deal with nested if’s is generally to move code into methods. Another solutions is to use an ‘early return’. (if !valid b; return
instead of if valid then a else b
)
I would write this something like:
#!/usr/bin/ruby
require 'thor'
require 'json'
class ListTrends < Thor
VALID_API_KEY_RE = /^(?=.*[a-zA-Z])(?=.*[0-9]).{8,}$/
desc 'list_trends json', 'list out trends from JSON file'
option :api_key, aliases: '--api-key', type: :string, required: true
option :format, type: :string, desc: 'one line format'
option :no_country_code, aliases: '--no-country-code', desc: 'remove country code', type :boolean
def list_trends(keyword=nil)
# Check your options before reading the file
if !VALID_API_KEY_RE.match(options[:api_key])
puts "Invalid API Key, operation abort..."
exit(255)
end
json = File.read('trends_available.json')
trends_hash = JSON.parse(json)
keyword = keyword.to_s.downcase
trends_hash.each do |trend|
process_trend(trend)
end
end
private
def process_trend(trend)
return unless trend["country"].downcase.include?(keyword)
if options[:format]
output_formatted_trend(trend)
else
output_complete_trend(trend)
end
end
def output_formatted_trend(trend)
output = trend.values[0..2]
output.delete_at(1) if options[:no_country_code]
puts output.join(' ')
end
def output_complete_trend(trend)
trend.each do |k, v|
if v.is_a?(Hash)
v.each do |i, j| puts "Trend location type #{i}: #{j}" end
else
puts "Trend #{k}: #{v}"
end
puts ''
end
end
end
ListTrends.start(ARGV)
Another advantage being that most people can only understand a few lines of code at a time, short methods also make it easier to keep track of variable scope and giving them meaningful names makes it easier to understand code without needing comments.