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

Use java time classes inside Personnummer #60

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ullenius
Copy link
Contributor

@ullenius ullenius commented Feb 8, 2022

Type of change

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

Description

Replace use of String with Java's time classes Year and Month.

getFullYear() now returns a Year object.

Related issue

Motivation

  • Better OOP
  • Strong typing
  • Increased performance. Strings are slow and allocate lots of memory.

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.

@@ -45,10 +47,10 @@ public static Personnummer parse(String personnummer) throws PersonnummerExcepti


private final int realDay;
private final String fullYear;
private final Year fullYear;
private final String century;
private final String year;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The property name year may cause confusion. Perhaps decade is a better name?

String getDecade()
Year getYear()

@Johannestegner
Copy link
Member

Hi @ullenius,
Thank you for the pull request!

I agree with the structure change and think your reasoning is correct, but I must take it up with the team to make sure we don't wander off too much from the core interface.
Will get back to you asap! :)

@ullenius
Copy link
Contributor Author

ullenius commented Feb 9, 2022

@Johannestegner
No worries. I was thinking of changing the getMonth()-method to return a Month object.

But I'd rather keep the PR small as this is a breaking change.

Storing the day and month inside a MonthDay object inside the class is also a possibility:

private final MonthDay monthDay;

public int getDay() { // breaks interface but is super fast
	return monthDay.getDay();
}

public Month getMonth() {
	return monthDay.getMonth();
}

@cbhat5 cbhat5 mentioned this pull request Mar 12, 2023
9 tasks
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