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

Added "separators" customization, NULL pre-checks, and Javadoc comment blocks to all constructors #63

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

Conversation

randolf
Copy link

@randolf randolf commented Jan 16, 2019

Added a new Hashids constructor to support customized "separators" plus a pre-check for NULL that prevents a NullException by defaulting to DEFAULT_SEPS (this behaviour is modeled after the "salt" pre-check for NULL).

Added a pre-check for NULL for the custom "alphabet" that prevents a NullException by defaulting to DEFAULT_ALPHABET (this behaviour is also modeled after the "salt" pre-check for NULL).

Added rudimentary comments for existing constructors for better Javadoc output. Added relevant details about the new constructor that supports custom "separators" (should this also be mentioned in the documentation on the web site?).

…t blocks to all constructors

Added a new Hashids constructor to support customized "separators" plus a pre-check for NULL that prevents a NullException by defaulting to DEFAULT_SEPS (this behaviour is modeled after the "salt" pre-check for NULL).

Added a pre-check for NULL for the custom "alphabet" that prevents a NullException by defaulting to DEFAULT_ALPHABET (this behaviour is also modeled after the "salt" pre-check for NULL).

Added rudimentary comments for existing constructors for better Javadoc output.  Added relevant details about the new constructor that supports custom "separators" (should this also be mentioned in the documentation on the web site?).
public Hashids(String salt, int minHashLength, String alphabet) {
this(salt, minHashLength, DEFAULT_ALPHABET, DEFAULT_SEPS);

Choose a reason for hiding this comment

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

alphabet isn't used here.

this.salt = salt != null ? salt : DEFAULT_SALT;
this.minHashLength = minHashLength > 0 ? minHashLength : DEFAULT_MIN_HASH_LENGTH;
if (alphabet == null) alphabet = DEFAULT_ALPHABET;
if (seps == null) seps = DEFAULT_SEPS;

Choose a reason for hiding this comment

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

Should follow the pattern above that was used for salt i.e.

this.alphabet = alphabet != null ? alphabet : DEFAULT_ALPHABET;

@0x3333
Copy link
Collaborator

0x3333 commented May 20, 2021

@randolf , are you still able to fix those issues raised by @Jon-Hopper?

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.

3 participants