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

What is limit and also it's broken #13

Open
whyboris opened this issue May 10, 2019 · 2 comments
Open

What is limit and also it's broken #13

whyboris opened this issue May 10, 2019 · 2 comments

Comments

@whyboris
Copy link
Contributor

whyboris commented May 10, 2019

There seems to be an extra parameter, limit, that is allowed:

module.exports = function(__this, that, limit) {

Yet if you ever put in a number that is smaller than the total steps the method fails:

e.g.

levenshtien("abc", "def")    // OK
levenshtien("abc", "def", 5) // OK    (since `steps` is 3, and 3 < 5)
levenshtien("abc", "def", 3) // OK    (since `steps` is 3, and 3 = 3)
levenshtien("abc", "def", 2) // fails (since `steps` is 3, and 3 > 2)

results in:

TypeError: Cannot read property '0' of undefined
      at module.exports (index.js:46:29)

I can't figure out what it was meant to accomplish. 🤔

Perhaps remove it altogether?

@tad-lispy
Copy link
Owner

Hm... Very good find. Thanks.

I suppose it is meant to prevent very long and expensive computations. Imagine you have a large collection of strings, perhaps every word in a given language or DNA sequences. You want to filter only similar ones (for spell checking or something like that). So you may say that word is similar when the distance is no more than 4 steps. To optimize the process you want to set a limit - once a given string reaches fifth step you want to terminate the computation and move on.

So I think the limit should stay, but it should not throw an error. That's a bug and we need to fix it.

My quick idea is to return:

{
  steps: undefined,
  similarity: undefined,
  relative: undefined
}

In this case undefined seems like a proper value - we don't know how many steps, or how similar it is so it is literally undefined. It should also work with the above use case (filtering), because undefined < 10 === false and undefined > 10 === false. So for example:

very_long_array_of_words.filter((word) => { 
  return levenshtien(word, "nosorożec", 5).steps < 5 
})

Also Array.prototype.sort puts undefined after any number, so if you would sort by steps or similarity, the ones outside of the limit will be treated as least similar.

Would it be difficult to annotate it with TS? I think it should be possible: either Number or undefined.

But that's just a very quick idea. What do you think? What is your use case?

@whyboris
Copy link
Contributor Author

I don't have a use case -- I was just looking at the code and was surprised to see an undocumented argument 🤷‍♂

I'll be happy to document the feature in the README as well as fix its behavior somehow (I think returning undefined is an elegant way to communicate to the module's user that something is wrong.

I'll look into it after #14 and #15 settle 😁

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

No branches or pull requests

2 participants