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

Improve String-to-Number Conversion Logic in Seed Handling #76

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

Conversation

allquantor
Copy link

@allquantor allquantor commented Oct 10, 2023

This Pull Request introduces an optimized and potentially more collision-resistant string-to-number conversion method in the seed-handling logic.

Before:
With the current logic, different string seeds might result in the same numeric seed if the sum of their character codes is the same. For example:

"AB" would be converted to 138 because 'A' has a character code of 65, ' B' has a character code of 66, and 65 + 66 + 1 (the initial accumulator value) is 132.
"BA" would also be converted to 132 for the same reason.

Example:

const seed = "BA"; // Try changing this to "AB" and you'll get the same result
const numberFromString = seed.split('').reduce((acc, curr) => acc + curr.charCodeAt(0), 1);
const numericSeed = Math.floor(Number(numberFromString));
console.log(numericSeed); // Output: 132

Now:
A straightforward improvement to the hashing function enhances its capacity to disperse different input strings to a broader range of output values, thus reducing collisions.

@alekmeckaroski
Copy link

alekmeckaroski commented Feb 7, 2024

@andreasonny83
Is this PR merging?
I am using this library for generating names and I am using unique strings as seed.
Although I am working with large number of seeds, inevitably coming to a collision I want to make them minimal.
Please review the code and merge it so I can update.

Thanks

@andreasonny83
Copy link
Owner

@alekmeckaroski
Please make sure to update the tests and write new use cases for covering the change introduced

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