4
\$\begingroup\$

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)
\$\endgroup\$
  • \$\begingroup\$ Could you please include an excerpt of the JSON and the corresponding output? \$\endgroup\$ – 200_success Mar 22 '17 at 13:08
2
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.