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

fix: calculate personnummer correctly before annual birthday #104

Merged
merged 1 commit into from
Oct 4, 2024

Conversation

flennic
Copy link
Contributor

@flennic flennic commented Oct 4, 2024

Type of change

  • New feature
  • Bug fix
  • Security patch
  • Documentation update

Description

Fixes an issue where the age is one year too much, if the person did not have had their birthday in the current year.

Related issue

Motivation

Less bugs, more happy people :)

Checklist

  • I have read the CONTRIBUTING document.
  • I have read and accepted the Code of conduct
  • Tests passes.
  • Style lints passes.
  • Documentation of new public methods exists.
  • New tests added which covers the added code

Notes

  • As you can see, I have not added a unit test for the fix. The reason for that is that the code uses DateTime.Now which will make the tests for this test case break depending on if the birth date has already taken place during the current year or not. The proper fix would be to use TimeProvider introduced in .NET 8, but I do not know the implications of that when targeting specific .NET versions and I do not have the time to look into it.
  • DateTime.Now has another issue, as it takes the local time of the computer running on. Personal Numbers are given according to the Swedish time zone, so there might be some edge cases when a computer runs in a time zone which is considered a different day than in Sweden. Still, this PR improves the accuracy from up to 366 days of error to 1 day.
  • I have used var. If you prefer the type instead, let me know and I will adjust the coding style.

{
var now = DateTime.Now;
var differenceInTotalMonth = 12 * (now.Year - Date.Year) + now.Month - Date.Month;
return differenceInTotalMonth / 12;
Copy link
Member

Choose a reason for hiding this comment

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

This will handle the month, but not the day, so we might diff on up to 30 days on a month.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very true, I somehow completely forget the days :D Will push an update soon.

@Johannestegner
Copy link
Member

DateTime is quite an issue, using the offset version or binding it to a timezone would ease things like this a lot, but the current thought is to leave it to the system running it (especially seeing that the age parameter is unlikely to be used in critical scenarios where a couple of hours would be a huge issue).

When it comes to the unit tests, the currently implemented ones might need a slight update, as they basically do the same mistake as the PR fixes, they will work today, but might break on some specific days.

@flennic
Copy link
Contributor Author

flennic commented Oct 4, 2024

I have (force) pushed an update. The result (given that today is 4. October 2024) now is:

Console.WriteLine(new Personnummer.Personnummer("19841003-7652").Age);
Console.WriteLine(new Personnummer.Personnummer("19841004-7651").Age);
Console.WriteLine(new Personnummer.Personnummer("19841005-7650").Age);
Console.WriteLine(new Personnummer.Personnummer("19841102-7652").Age); // Larger month, smaller day
39
40
40
40

I implemented it in this way because any counting of days would end up in endless leap year issues.

Copy link
Member

@Johannestegner Johannestegner left a comment

Choose a reason for hiding this comment

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

LGTM!

thanks for the fix :)

@Johannestegner Johannestegner merged commit bd6d979 into personnummer:master Oct 4, 2024
5 checks passed
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