Fix knuthMorrisPratt for empty word request #101
Conversation
Codecov Report
@@ Coverage Diff @@
## master #101 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 115 115
Lines 2255 2254 -1
Branches 393 391 -2
=====================================
- Hits 2255 2254 -1
Continue to review full report at Codecov.
|
| @@ -2,6 +2,9 @@ import knuthMorrisPratt from '../knuthMorrisPratt'; | |||
|
|
|||
| describe('knuthMorrisPratt', () => { | |||
| it('should find word position in given text', () => { | |||
| expect(knuthMorrisPratt('', '')).toBe(0); | |||
| expect(knuthMorrisPratt('a', '')).toBe(0); | |||
| expect(knuthMorrisPratt('a', 'a')).toBe(0); | |||
There was a problem hiding this comment.
This test is already valid without main function code changes.
| @@ -2,6 +2,9 @@ import knuthMorrisPratt from '../knuthMorrisPratt'; | |||
|
|
|||
| describe('knuthMorrisPratt', () => { | |||
| it('should find word position in given text', () => { | |||
| expect(knuthMorrisPratt('', '')).toBe(0); | |||
| expect(knuthMorrisPratt('a', '')).toBe(0); | |||
There was a problem hiding this comment.
This test is actually not correct. It says that when I search for an empty word '' in a string 'a' I should find one and this empty word will be accessible at zero index. But string[0] will return us a which expected but is not correct according to this test. So I assume that we should expect -1 here instead of 0.
There was a problem hiding this comment.
@trekhleb Actually I was understanding it a bit differently.
In my understanding I was expecting knuthMorrisPratt to return the same as String.prototype.indexOf - more a less in a sense.
In other word I considered the output as being: index within text of the first occurrence of word [official reference].
// definition in terms of code
const startIdx = knuthMorrisPratt(text, word);
text.substr(startIdx, word.length) === word // should be true for startIdx !== -1Actually both "".indexOf("") === 0 and "a".indexOf("") === 0.
| @@ -2,6 +2,9 @@ import knuthMorrisPratt from '../knuthMorrisPratt'; | |||
|
|
|||
| describe('knuthMorrisPratt', () => { | |||
| it('should find word position in given text', () => { | |||
| expect(knuthMorrisPratt('', '')).toBe(0); | |||
There was a problem hiding this comment.
I think this test is wrong. We should expect -1 here instead of 0 because having zero as a result would mean that I may fetch empty word out of string[0]. But this is not true since the string itself is empty.
| @@ -30,6 +30,10 @@ function buildPatternTable(word) { | |||
| * @return {number} | |||
| */ | |||
| export default function knuthMorrisPratt(text, word) { | |||
| if (word.length === 0) { | |||
There was a problem hiding this comment.
Please take a look at the comments to the tests above. No need to do this checking.
|
@dubzzz thank you for you PR. Could you please check my comments in code to this PR though. It seems like tests are correct now. It is ok to receive |
|
Actually both console.log(rabinKarp('', '')); //0
console.log(rabinKarp('a', '')); //0
console.log(rabinKarp('a', 'a')); //0
console.log(''.indexOf('')); //0
console.log('a'.indexOf('')); //0
console.log('a'.indexOf('a')); //0I can re-open the PR if you agree on it ;) |
While running property based tests - based on fast-check - on the implementation of knuthMorrisPratt I discovered what seems to be an inconsistency:
The tests that discovered the issue are available in the commit: dubzzz@87329dd
If you are interested in this test method, I can issue another pull request for those property based tests.