hockdudu
commented
2 years ago I think it's worth mentioning: When refactoring the code I noticed the UV index didn't change to the new city immediately, only after manually refreshing. With the changes in the MainActivity (function parseTodayJson) this shouldn't be of a problem anymore.
I believe this is related to https://github.com/martykan/forecastie/pull/622, to https://github.com/martykan/forecastie/pull/644 and to https://github.com/martykan/forecastie/issues/612.
@BAESI FYI
robinpaulson
igor-cali
FridoDeluxe
This merge request has the main goal of using a centralized weather storage for reading and saving the current weather cache.
Background
There's an issue open for a long time already: Support for multiple locations #76. This is a feature I'd love to see, as well as many others would also like, as it can be found in some Google Play reviews.
I've tried in the past to implement this feature, but I couldn't finish it because the number of required changes is too big. What made it more difficult, is that I had to not only do the changes for the feature itself, but also had to change numerous other places for it to work well together. This is more of less the "shotgun surgery" antipattern.
As I suggested in a comment in the aforementioned issue, I believe the current codebase needs some changes before the multiple locations can be implemented, otherwise there would be too many changes to keep track of, making both the developing and reviewing process more difficult, leading to an increased risk of introducing bugs. This pull request is therefore the first step into implementing the multiple locations.
But I believe this pull request doesn't only benefit the implementation of the multiple locations, but the codebase in general, as it improves the code quality by improving the separation of concerns.
Main changes
The main change is the use of the WeatherStorage class. This class is used for reading and saving important weather data, such as current weather, long-term weather, UV index and geolocation.
Secondarily, this pull request moves most of the parsing to a separate class, the OpenWeatherMapJsonParser. For this class, I also added unit tests. But here I must add, unit tests are not my strength, so it might need some reviewing.
Thirdly, I also improved a few types for the Weather class, e.g. using integers instead of strings for storing numbers.
One part I didn't touch in this pull request are the notifications and the immutable weather. I think changes here aren't required yet, so I wanted to avoid making this pull request too big.