Minor refactor #8
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Methods `noun_suffix_machine` and `derivational_suffix_machine` did not need to take a block (containing a single word) and yield. They weree refactored to take the word as an argument.
cmantas
force-pushed
the
minor_refactor
branch
6 times, most recently
from
1560434 to
5cafb32
Compare
All the stem candidates are appended to the list and uniq! is called once.
greenonion
requested changes
Mar 14, 2017
lib/turkish_stemmer.rb
Outdated
|
|
||
| # Postprocess | ||
| # set of stem candidates | ||
| stems = [original_word, *nominal_verbs_suffix_machine(original_word.dup)] |
There was a problem hiding this comment.
While I agree that this code can be simplified, I'm not sure I agree with this approach:
- I like the step by step approach of the original. I think it enhances readability.
- I agree that calls to
flatten!can be replaced, though I prefer the+array operator to splatting. - Calling
uniq!before mapping the array to thenoun_suffix_machinemethod makes sense. - I think calls to
&methodis a bit of a code smell (and I think hinder performance a bit as well, see here).
As a final thought I don't like the need for using dup on the original before calling methods that are not explicitly mutating. Ideally I think these methods should call dup themselves on their arguments. Otherwise I think they should have a bang ! operator in their names.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Minor refactor of the
stemmethod.