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

Using Kotlin require function to check for number range and maximum attempts #3

Merged
merged 2 commits into from
Dec 31, 2023

Conversation

jelle82
Copy link

@jelle82 jelle82 commented Dec 27, 2023

Kotlin has some nice functions for checking and throwing exceptions based on a predicate.

I think the proposed changes makes the code more idiomatic Kotlin and is more concise.

@4kimov 4kimov requested a review from kevinxmorales December 27, 2023 21:58
@jelle82
Copy link
Author

jelle82 commented Dec 27, 2023

Even if this pull request is not merged:

There might be some a test case not covered, when I change this line require(increment <= alphabet.length) { "Reached max attempts to re-generate the ID" } into require(increment < alphabet.length) { "Reached max attempts to re-generate the ID" }, all the tests still pass.

I could look into this also, if you want and try to write a test for it.

@kevinxmorales
Copy link
Collaborator

Even if this pull request is not merged:

There might be some a test case not covered, when I change this line require(increment <= alphabet.length) { "Reached max attempts to re-generate the ID" } into require(increment < alphabet.length) { "Reached max attempts to re-generate the ID" }, all the tests still pass.

I could look into this also, if you want and try to write a test for it.

This check directly ported from the sqids specification
The maxBlockList test is a good example of what this limit aims to do, try as hard as possible to continually produce new ids in the case that the block list prevents a generated id from being used. In the example you gave, we would give up on trying to produce the id 1 less times than we normally would. I think to properly test that, you'd need to have an alphabet of length x and a block list of length x - 1 to see the importance of the line as it is written. I think its worth a test case both here and in the sqids-spec.

Copy link
Collaborator

@kevinxmorales kevinxmorales left a comment

Choose a reason for hiding this comment

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

This change is great! If you notice anything else that might not be idiomatic Kotlin, feel free to make more changes. Highly appreciated

@kevinxmorales kevinxmorales merged commit a2741e1 into sqids:main Dec 31, 2023
1 check passed
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.

2 participants