Boyer-Moore algorithm updates #330
Conversation
There was a problem hiding this comment.
Great work 👍
Thanks for the improvements. I think the changes you've made works and looks great.
I took some time to review the rest of the codebase and I found some things that can be improved upon. I left some comments, let me know what you think of them.
Cheers!
| @@ -6,7 +6,7 @@ | |||
| http://www.drdobbs.com/database/faster-string-searches/184408171 | |||
| */ | |||
| extension String { | |||
| func indexOf(pattern: String) -> String.Index? { | |||
| func indexOf(pattern: String, useHorspoolImprovement: Bool = false) -> String.Index? { | |||
There was a problem hiding this comment.
I think index(of pattern: String, useHorspoolImprovement: Bool = false) -> Index? is a better method signature.
Since Swift 3, many methods switched over from indexOf to index(of:). String.Index is unnecessary since we're inside a String extension and the compiler can infer the type of Index.
| // If no match, we can only safely skip one character ahead. | ||
| i = index(after: i) | ||
| } | ||
| else { |
There was a problem hiding this comment.
Our style guidelines prefer to have the else statement on the same line as the closing brackets for the if statement.
| // Cache the length of the search pattern because we're going to | ||
| // use it a few times and it's expensive to calculate. | ||
| let patternLength = pattern.characters.count | ||
| assert(patternLength > 0) |
There was a problem hiding this comment.
I feel like we should replace these asserts with guard patternLength > 0, characters.count < patternLength else { return nil }. What do you think?
| // The pattern is scanned right-to-left, so skip ahead in the string by | ||
| // the length of the pattern. (Minus 1 because startIndex already points | ||
| // at the first character in the source string.) | ||
| var i = self.index(startIndex, offsetBy: patternLength - 1) |
There was a problem hiding this comment.
Unnecessary reference to self. There are a couple of these.
|
Thanks for the comments! I agree with the improvements and implemented the changes. Also updated the playground and made other minor changes (added tests, renamed The new Swift 3 style for method signatures feels more natural. Regards! |
| } | ||
| func index(of pattern: String, usingHorspoolImprovement: Bool = false) -> Index? { | ||
| // There are no possible match in an empty string | ||
| guard !isEmpty else { return nil } |
There was a problem hiding this comment.
This feels a bit redundant since the latter guard statement covers this case as well.
| func indexOf(pattern: String) -> String.Index? { | ||
| func index(of pattern: String, usingHorspoolImprovement: Bool = false) -> Index? { | ||
| // There are no possible match in an empty string | ||
| guard !isEmpty else { return nil } |
There was a problem hiding this comment.
Same as before, the latter guard statement should cover this case just as well.
|
You're right. Redundant validation removed. 👍 |
|
Merging this in! Thanks and happy Boxing Day! @mmazzei |
This is what I did:
This fixes #284.