Display random facts about cities

Question

This is one of the first scripts I've written. I set it aside for a month or so and I'm not sure how to go about refactoring it. I'd love to have it be more OOD, but I have much more studying before that, I'm afraid.

require_relative 'data_values' #the strings of facts for each city are stored in an array here
 #any string in the hash is just a 'fill-in' for cities without facts yet.
city_hash = {  
:Portland => [FactPort0 , FactPort1 , FactPort2, FactPort3 , FactPort4],
:SanFrancisco => [FactSF0 , FactSF1 , FactSF2, FactSF3 , FactSF4] ,
:SantaFe => [FactSAFE0 , FactSAFE1 , FactSAFE2, FactSAFE3 , FactSAFE4]   ,
:Tokyo => [FactTKY0 , FactTKY1 , FactTKY2, FactTKY3 , FactTKY4] ,
:Seattle => [FactSEA0 , FactSEA1 , FactSEA2, FactSEA3 , FactSEA4] ,
:Vancouver => [FactVAN0 , FactVAN1 , FactVAN2, FactVAN3 , FactVAN4] ,
:LakeTahoe => [FactTAH0 , FactTAH1 , FactTAH2, FactTAH3 , FactTAH4] ,
:Cambridge => [FactCAM0 , FactCAM1 , FactCAM2, FactCAM3 , FactCAM4] ,
:Berlin => [FactBER0 , FactBER1 , FactBER2, FactBER3 , FactBER4] ,
:SantaClara => [FactSC0 , FactSC1 , FactSC2, FactSC3 , FactSC4] ,
:Miami => [FactMIA0 , FactMIA1 , FactMIA2, FactMIA3 , FactMIA4] ,
:Beijing => [FactBEI0 , FactBEI1 , FactBEI2, FactBEI3 , FactBEI4] ,
:LosAngeles => [FactLA0 , FactLA1 , FactLA2, FactLA3 , FactLA4] ,
:Prague => [FactPRA0 , FactPRA1 , FactPRA2, FactPRA3 , FactPRA4] ,
:Austin => [FactAUSTX0 , FactAUSTX1 , FactAUSTX2, FactAUSTX3 ,     FactAUSTX4] ,
:Durham => ["FactDUR0" , "FactDUR1" , "FactDUR2", "FactDUR3" , "FactDUR4"] ,
:Seville => ["FactSEV0" , "FactSEV1" , "FactSEV2", "FactSEV3" , "FactSEV4"] ,
:Yokohama => ["FactYOKO0" , "FactYOKO1" , "FactYOKO2", "FactYOKO3" , "FactYOKO4"] ,
:Bengaluru => ["FactBENG0" , "FactBENG1" , "FactBENG2", "FactBENG3" , "FactBENG4"] ,
:SanJose => ["FactSJ0" , "FactSJ1" , "FactSJ2", "FactSJ3" , "FactSJ4"]
 }

list = city_hash.keys.sort
error_message = "\n Factbot didn't understand. Check your typing and   try again. \n"
end_prompt = "Thanks for using fact_bot!"

puts "Welcome to fact_bot! fact_bot knows some facts about the     following cities: \n "
puts list << "\n"
#none of the regex's are complete, most aren't 'functional' mainly filler!
loop do
puts "| Type any city listed to see facts |'exit' to leave | 'list' to    view cities |  "
gets
case $_.downcase
when /^exit/
  break
when /^list/
  $_ = list
when /aus.+|.+tin|.+ust.+|.+sti.+|a.+t.+in/                   #austin
  $_ = :Austin                                              
when /ji/                                                     #beijing
  $_ = :Beijing
when /al|b.n|ga|lu|ru/                                           #bengaluru
  $_ = :Bengaluru
when /b.r|rl/                                                 #berlin
  $_ = :Berlin
when /ca|^c|dg|mb|ri/                                         #cambridge
  $_ = :Cambridge    
when /du|m$|rh/                                               #durham
  $_ = :Durham
when  /ah|ak|et|ho|ke|oe/                                     #lake tahoe
  $_ = :LakeTahoe
when /s$/                                                     #los angeles
  $_ = :LosAngeles
when /m.+m/                                                   #miami
  $_ = :Miami    
when /nd|or|po|p.+d|rt/                                       #portland
  $_ = :Portland
when /ag|gu|pr|p.+e/                                          #prague
  $_ = :Prague 
when /c.+c|fr|nf|n.f|sc|sf/                                   #san francisco
  $_ = :SanFrancisco
when /jo|nj|n.j/                                              #san jose
  $_ = :SanJose
when /ac|ar|cl/                                               #santa clara
  $_ = :SantaClara
when /af|fe/                                                  #santa fe
  $_ = :SantaFe
when /at|sea|tt/                                              #seattle
  $_ = :Seattle
when  /s.+v/                                                  #seville
  $_ = :Seville
when /ky|to/                                                  #tokyo
  $_ = :Tokyo
when /r$/                                                     #vancouver
  $_ = :Vancouver
when /ko|ma/                                                  #yokohama
  $_ = :Yokohama
else
  $_ = error_message
end
  if $_ == error_message
    puts error_message
  elsif $_ == list
    puts list
  else
    puts "\n Random #{$_} Fact: #{city_hash[$_].sample(1)} \n\n" 
  end
end
puts end_prompt

I feel like I have my work cut out for me.... I'm really struggling to make regex's for city names and accounting for typo's etc...surely I'm overthinking that.

I feel as there is a better way of calling the facts? I don't like using $_ it doesn't feel ruby? The massive Case statement feels especially horrible and not DRY.

I'm looking for a more Ruby solution to the script. This feels very forced even if it does 'work'.

Answer 1

Here's a more OOP way. I've taken liberties and changed the code a little bit - added some new lines etc. but overall the basic functionality of both programs are the same. I used TDD to get the thing going and then abondoned it because it was faster to just type it out.

please note that i've employed polymorphism and have created a city class. and i have subclasses for each different type of city. the facts are located within each of my unique city classes.

The code is pretty self explanatory. the downside is that i have two inifinite loops in the code - and i hate those things - i tried and avoid that if at all possible, but it's difficult to avoid here given that's what OP wants the code to be doing.

another thing to note is that the code hardly has any conditionals -- apart from that which exists in the "factory method" - where it should be. this is where we choose which city should be instantiated. (A factory method is a method which instantiates an object of your choosing). the other conditionals of course are associated with the infinite loops i created.

this code it will help you to see things in a more OOP way. it should be very easy to read and understand - compare the readability to your attempt. it should just feel a lot simpler and organised - perhaps even elegant. any questions, feel free to ask away, or refer to google. hope this helps :)

class Quiz
  attr_reader :cities

  def initialize
    @cities = get_cities   
    introduction
  end

  def get_cities
    ObjectSpace.each_object(Class).select { |klass| klass < City }.collect{ |c| c.new}
  end

  def introduction
    city_names = @cities.collect {|c| c.name }
    cities_joined = city_names.join("\n")
    puts "\n\n\nWelcome to fact_bot! fact_bot knows some facts about the following cities: \n#{cities_joined}"
  end

  def question   
    while true
      city = city_factory
      puts city.facts
    end
  end

  def city_factory
    while true 
      puts "Type any city listed to see facts |'exit' to leave | 'list' to view cities | \n"
      answer = gets.chomp     
      case answer.downcase
      when "sydney"
        return CitySYD.new
      when "san fransisco"
        return CitySF.new
      when "london"
        return CityLN.new
      when "exit"
        abort
      when "list"
        introduction     
      else 
        puts "Spelling mistake - please type the city correctly\n"       
      end
    end
  end
end

class City
  attr_reader :name, :facts

  def initialize
    # if you had a database you could probably get the values with 
    # something like this.
    # but we don't have such a facility available to us
    # So i thought to employ polymorphism and a factory method
    # to get us the result we need
    # @name = get_name()
    # @facts = get_facts()
  end

  def get_name
    ""
  end

  def get_facts
    []
  end
end

class CitySF < City
  def initialize
    @name = "San Fransisco"
    @facts = get_facts()
  end

  def get_facts
    ["Population is 10m", "USA"]
  end
end

class CityLN < City
  def initialize
    @name = "London"
     @facts = get_facts()
  end

  def get_facts
    ["Population is 20m", "England, UK"]
  end
end


class CitySYD < City
  def initialize
    @name = "Sydney"
     @facts = get_facts()
  end

  def get_facts
    ["Population is 4m", "Australia"]
  end

end

q = Quiz.new
q.question

Answer 2

I think the main thing you should do is move your regexes out of your main loop. You could create a city class if you want, but if you're not ready for that, you could make another hash, say city_regex, that maps city name to regex. Then the heart of your main loop becomes:

reply = gets
case reply.downcase
when /^exit/
  break
when /^list/
  puts list
else
  found = city_regex.find {|k,v| reply =~ v }
  if found.empty?
    puts error_message
  else
    name = found[0]
    puts "\nRandom #{name} Fact: #{city_hash[name].sample(1)} \n\n"
  end
end

Notice how when I do this, the code becomes clearer. Instead of having a long list of cities and regexes, the code just searches for a matching city, which is really what you're trying to do.

I also switched from using the special global variables to using local variables. The advantage of local variables is you can convey some meaning to whoever is reading your code, even if you're the only person reading it.