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 NullPointerException in TextUtil.compare #4577

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

Conversation

OnkelDok
Copy link
Member

@OnkelDok OnkelDok commented Mar 6, 2025

https://forum.portfolio-performance.info/t/fehlermeldung-internal-error/32110

I dont know why, but it seems at least one security name is null. So I added a global fix for all users of TextUtil.compare and also created simple unittest.


One additional point I thought about: In the code there are plenty of this nullchecks for comparisons (e.g. https://github.com/portfolio-performance/portfolio/blob/master/name.abuchen.portfolio/src/name/abuchen/portfolio/model/Security.java#L53-L58).
Would it be helpful/better to create a commonly used method where this nullcheckss are executed and use this everywhere?
Of course this is only possible by using Optional to also realize the path when both objects/values are not null.
Does that make sense?

Copy link
Member

@Nirus2000 Nirus2000 left a comment

Choose a reason for hiding this comment

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

Hello @OnkelDok
Awesome thing from once again... 😲
I read your entry and think about it for a moment and then the aha! moment... 👀
Maybe the change fits better... take a look at it at your leisure.

Geil 👍🏻

Regards
Alex

Comment on lines +328 to +333
if (left == null && right == null)
return 0;
else if (left == null)
return -1;
else if (right == null)
return 1;
Copy link
Member

@Nirus2000 Nirus2000 Mar 7, 2025

Choose a reason for hiding this comment

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

Suggested change
if (left == null && right == null)
return 0;
else if (left == null)
return -1;
else if (right == null)
return 1;
if (left == right) return 0; // Both are null or refer to the same object
if (left == null) return -1;
if (right == null) return 1;

Comment on lines +197 to +199
assertEquals(-1, TextUtil.compare(null, "a"));
assertEquals(1, TextUtil.compare("a", null));
assertEquals(0, TextUtil.compare(null, null));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
assertEquals(-1, TextUtil.compare(null, "a"));
assertEquals(1, TextUtil.compare("a", null));
assertEquals(0, TextUtil.compare(null, null));
assertEquals(-1, TextUtil.compare(null, "a"));
assertEquals(1, TextUtil.compare("a", null));
assertEquals(0, TextUtil.compare(null, null));
assertEquals(0, TextUtil.compare("abc", "abc"));
assertTrue(TextUtil.compare("a", "b") < 0); // "a" should be less than "b"
assertTrue(TextUtil.compare("z", "y") > 0); // "z" should be greater than "y"
assertTrue(TextUtil.compare("abc", "ABC") != 0); // Depends on COLLATOR
assertTrue(TextUtil.compare("café", "cafe") != 0); // Accents matter

Copy link
Member Author

@OnkelDok OnkelDok Mar 7, 2025

Choose a reason for hiding this comment

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

Interesting idea. I don't thought about it before. Maybe for strings this is fine. But I think I want only check for null and let the underlaying comparer make the decision for not-null objects.
It's up to @buchen what to use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

2 participants