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

String property can be empty #782

Merged

Conversation

samalloing
Copy link
Collaborator

Hi @carlwilson

In PDF files the String Properties like Title can be empty. Now the value is null and will give a nullpointerexception when that happens. I updated the jHove Property, but I could also only update the addStringProperty in PDF module.

Sam

@carlwilson carlwilson added this to the JHOVE 1.28 milestone Nov 30, 2022
@carlwilson carlwilson self-assigned this Nov 30, 2022
if (type == PropertyType.STRING) {
// a String value can be empty
value = "";
} else {
Copy link
Member

Choose a reason for hiding this comment

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

The else is unnecessary if it's not a string then we always want the exception which terminates.

throw new NullPointerException (CoreMessageConstants.EXC_PROP_VAL_NULL);
if (type == PropertyType.STRING) {
// a String value can be empty
value = "";
Copy link
Member

Choose a reason for hiding this comment

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

Look at the Codacy feedback. It might be a good idea not to reassign value and add an intermediary value instead. Parameter mutation is rarely a good thing ;)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @carl, can you look at the new proposal I created for this? 9181e61. The code doesn't become better readable, but it does what I wanted to achieve with this pull request without reassigning. I tried different options, but there was always some reassignment happening.

@carlwilson carlwilson merged commit c80612f into openpreserve:integration Jan 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants