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

Wrote fixDateTime() method #185

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

Conversation

Harrison-O
Copy link

Wrote fixDateTime() method: attempts to fix invalid DateTime object by decrementing invalid time/date components and incrementing next components to compensate. E.g. If DateTime object represents the 32 July 2019 then running fixDateTime() will change it to 1 August 2019.

Also wrote getDaysInMonth() and isLeapYear() functions. These are used by fixDateTime() hence the inclusion.

Copy link
Contributor

@edgar-bonet edgar-bonet left a comment

Choose a reason for hiding this comment

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

I didn't test it yet, but I fear that the various += operations are prone to overflow. Maybe the intermediate calculations should be done on 16-bit local variables.

RTClib.cpp Outdated
// make month valid to prevent getDaysInMonth() returning 0
// Otherwise, infinite loop possible
if (m > 12) {
temp = m % 12;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is probably meant to be m / 12

Suggested change
temp = m % 12;
temp = m / 12;

Copy link
Author

Choose a reason for hiding this comment

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

fixed

RTClib.cpp Outdated
m++;
if (m > 12) {
yOff++;
m--;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be m = 1 or m -= 12

Suggested change
m--;
m -= 12;

Copy link
Author

Choose a reason for hiding this comment

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

fixed

@edgar-bonet
Copy link
Contributor

edgar-bonet commented Jul 30, 2020

I did a small test that shows the effect of the mm field overflowing. fixDateTime() performs the following conversions:

2000-01-01T00:255:59 (INVALID) -> 2000-01-01T04:15:59 (valid)
2000-01-01T00:255:60 (INVALID) -> 2000-01-01T00:00:00 (valid)

The first one is the correct, expected behavior. The second one is incorrect: it should return 2000-01-01T04:16:00.

Edit: the case when either the month or the day is zero is also mishandled:

2020-00-01T00:00:00 (INVALID) -> 2020-01-01T00:00:00 (valid)
2020-01-00T00:00:00 (INVALID) -> 2020-01-00T00:00:00 (INVALID)

Month 0 should presumably be understood as December from the previous year, but fixDateTime() simply replaces it with January, without touching the year. Day 0 is left as is, which results in the “fixed” DateTime being invalid.

As a side note, I find the name of the method (DateTime::fixDateTime()) a bit redundant.

RTClib.cpp Outdated
*/
/**************************************************************************/
bool isLeapYear(uint16_t year) {
return year % 400 == 0 || (year % 4 == 0 && year % 100 != 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Within the range of supported years (2000–2099), year % 4 == 0 is enough. And this test is already used in a couple of places in the current code.

Copy link
Author

Choose a reason for hiding this comment

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

Simplified isLeapYear() and made it static to prevent misuse.

Fixed bug with `fixDateTime()` overflowing DateTime data members.

Simplified `isLeapYear()` function for efficiency at the cost of not
working for years of multiples of 100 and not multiples fo 400. Also
wrote warning for limitation.

Made `isleapYear()` static to avoid misuse.

Rewrote warning for `fixDateTime()` to specify that the year becomes
2100 if fixing DateTime requires setting the year to before 2000.
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