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

Streamline UUIDConverter #8766

Merged
merged 3 commits into from
Oct 16, 2023
Merged

Conversation

tzolov
Copy link
Contributor

@tzolov tzolov commented Oct 16, 2023

  • Use StandardCharsets.UTF_8 as a charset configuration instead of string. Later remove the necessity of handling encoder errors.
  • Use regular expressions to validate the UUID string standard representation. Later obsolete the need for try/catch exceptions.

Copy link
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

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

Thank you for quick turn around!
I think we can accept this change and back-port it respectively.
Just see if my review makes sense!

*/
public class UUIDConverter implements Converter<Object, UUID> {

public static final String DEFAULT_CHARSET = "UTF-8";
public static final Charset DEFAULT_CHARSET = StandardCharsets.UTF_8;
Copy link
Member

Choose a reason for hiding this comment

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

We cannot do this change since it is a public property.
Since it is final and really cannot be changed, I suggest the change like:

  1. Deprecate this constant: there is really just that StandardCharsets.UTF_8. This one in this class is just out of use.
  2. Use the mentioned StandardCharsets.UTF_8 in the code below instead of this constant.


/**
* Convert the input to a UUID using the convenience method
* {@link #getUUID(Object)}.
* Convert the input to a UUID using the convenience method {@link #getUUID(Object)}.
*
Copy link
Member

Choose a reason for hiding this comment

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

Consider to remove blank lines from method Javadocs since we are already here in changes for this class.

@@ -115,4 +103,14 @@ private static byte[] serialize(Object object) {
return stream.toByteArray();
}

/**
* Verifies if the provided UUID string complies with the standard representation.
Copy link
Member

Choose a reason for hiding this comment

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

I don't see a reason in Javadocs for private methods, but if you still would like to have it here, please, consider to make it consistent we are rules:

  1. Imperative (not infinitive) - therefore Verify.
  2. No blank lines in method Javadocs.

https://github.com/spring-projects/spring-framework/wiki/Code-Style#javadoc-formatting

 - Use StandardCharsets.UTF_8 as a charset configuration instead of string.
   Later remove the necessity of handling encoder errors.
 - Use regular expressions to validate the UUID string standard representation.
   Later obsolete the need for try/catch exceptions.
@artembilan artembilan merged commit 77d8dfd into spring-projects:main Oct 16, 2023
1 check passed
@artembilan
Copy link
Member

Subsequent commit does some clean up: ea8d917

Cherry-picked to 6.1.x & 6.0.x as 68f977a

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants