batoulapps / adhan-kotlin

High precision Islamic prayer time library for Java
MIT License
161 stars 47 forks source link

Re-construct DateComponents in UTC #15

Closed z3bi closed 4 years ago

z3bi commented 4 years ago

I noticed that in PrayerTimes.java the function public PrayerTimes(Coordinates coordinates, DateComponents date, CalculationParameters params) constructs a Date object by calling resolveTime which does this using a UTC calendar. Then in private PrayerTimes(Coordinates coordinates, Date date, CalculationParameters parameters) the date is turned back into components using DateComponents.from(date) but this assumes the local timezone. This doesn't work when it's run on a machine that isn't in UTC. To fix this I added fromUTC function to DateComponents so that the date components are correctly reconstructed.

codecov[bot] commented 4 years ago

Codecov Report

Merging #15 into master will decrease coverage by 0.79%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #15      +/-   ##
==========================================
- Coverage   95.20%   94.41%   -0.80%     
==========================================
  Files          20       20              
  Lines         501      501              
  Branches       53       53              
==========================================
- Hits          477      473       -4     
- Misses         16       20       +4     
  Partials        8        8              
Impacted Files Coverage Δ
...n/java/com/batoulapps/adhan/data/CalendarUtil.java 94.11% <ø> (ø)
...rc/main/java/com/batoulapps/adhan/PrayerTimes.java 91.47% <100.00%> (-0.05%) :arrow_down:
...rc/main/java/com/batoulapps/adhan/SunnahTimes.java 100.00% <100.00%> (ø)
...java/com/batoulapps/adhan/data/DateComponents.java 69.23% <100.00%> (-30.77%) :arrow_down:
...java/com/batoulapps/adhan/data/TimeComponents.java 95.00% <100.00%> (+0.55%) :arrow_up:
.../java/com/batoulapps/adhan/internal/SolarTime.java 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 6b7fed4...2332867. Read the comment docs.

z3bi commented 4 years ago

jazakumAllah khairan, great catch!

nitpick - what do you think (as an alternative fix) to just merge the two constructors instead - so removing the private constructor and computing date inside using CalendarUtil.resolveTime directly? this way we won't need the fromUTC method.

Yeah it looks like there are a few places where we seem to go back and forth from Date to DateComponents and we could cut down on that. I think we might still need fromUTC for doing things like converting to a Date object for the final prayer times.

Let's leave this PR open and I'll take a stab at consolidating some of the date conversion tasks. For example I made a change while I was refactoring the JavaScript code to no longer need to do calendar conversions in SolarTime as Julian day can just be incremented and decremented by 1 to get a value for tomorrow and yesterday https://github.com/batoulapps/adhan-js/blob/master/src/SolarTime.js

z3bi commented 4 years ago

@ahmedre ok I did some further consolidation of Date and DateComponents but we still need fromUTC since resolveTime goes through UTC and we require a Date object to do calendrical calculations to get tomorrow's date.

I think though that this is a bit more straightforward since the DateComponent that the function signature takes is the primary date object when calculating prayer times and we only go to a fully realized Date for a brief calculation of tomorrow and for assigning final prayer time Date objects.

ahmedre commented 4 years ago

jazakumAllah khairan - looks good to me