Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modified meteoservice.rb to parse cities and their indexes from json file #30

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

TimKm
Copy link

@TimKm TimKm commented Jun 26, 2020

As well fixed a bug which crashed the program in case if user enters wrong value second time.

BEFORE:
weather_forecast
AFTER:
fixed

TimKm added 2 commits June 26, 2020 17:50
added json file that contains the name of the cities with their corresponding index on (https://www.meteoservice.ru/content/export.html)
Cities with their corresponding indexes are parsed  from cities.json file now. It is done in order to modify the json file itself and not to tackle with the main code. As well this practice was taught in previous lessons.
Aside of that, changed statement "unless" to "until". With "unless" if the user enters wrong input second time then the program would continue and crash, while with "until" it will prompt user to enter the number again and again until the correct one is matched the condition.
Copy link
Owner

@aristofun aristofun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good improvement but inline CITIES back please. It’s important to keep newcomers simple.

Until they explicitly would need json, for example.


require_relative 'lib/meteoservice_forecast'
require_relative 'data/cities.json'
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it required?

please inline it for simplicity

puts "#{index+1}: #{name.first}"
end

city_index = gets.chomp.to_i
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No meed for chomp here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants