-
Notifications
You must be signed in to change notification settings - Fork 1
Remove stopwords refacture #4
base: master
Are you sure you want to change the base?
Conversation
mowolf
commented
May 27, 2018
- refactured removeStpwords
|
||
export const removeStopwords = (wordCountObject, language) => { | ||
var stopwords = STOPWORDS[language]; | ||
var filteredObject = Object.assign(wordCountObject); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this line do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This creates a new object. So that we don not mutate the original object, but instead have two separate ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but this is not what this line does. check Object.assign
method documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Object.assign() method is used to copy the values of all enumerable own properties from one or more source objects to a target object. It will return the target object.
Check out this codepen. Here I create a new object and then manipulate it without the first object being mutated.
https://codepen.io/mowolf/pen/pKjGyb?editors=0012
for (var key in wordCountObject) { | ||
var containsWord = (wordsToFilterOut.indexOf(key) > -1); | ||
if (containsWord) { | ||
delete wordCountObject[key]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as discussed, this is risky as it mutates the original object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comment above. Should be fine if this is a separate object.
src/removeStopwords.test.js
Outdated
expect( typeof removeStopwords ).toBe( 'function' ); | ||
} ); | ||
|
||
it( 'should remove stopords', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stopords
-> stopWords
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jep
src/removeStopwords.js
Outdated
// SUGGESTIONS: refacture ino shouldDisplay | ||
// and this function take a word and the language, then it returns true or false, and this function should be used wherever you are trying to display the words | ||
|
||
import { STOPWORDS } from './constants'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better to save STOPWORDS in a separate file as it is localization not code constants
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jep
} | ||
|
||
export const removeStopwords = (wordCountObject, language) => { | ||
var stopwords = STOPWORDS[language]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right now this function depends on (tightly coupled with) the actual words. so in your tests you have to use one of these words from the original stopwords.
to avoid this behavior, we could do one of:
- Mock
STOPWORDS
during the test (which is a bit complicated) - as
STOPWORDS
is a dependency that we can inject to this function and in tests we can inject different "simplified" words.
for example:
export const createRemoveStopwords =(stopwords)=> (wordCountObject, language) => {
// logic here ..
}
and then we export another function like that:
export const removeStopwords(STOPWORDS);
and when we test, we use custom/simple stop words:
+import { createRemoveStopwords } from './removeStopwords';
it( 'should remove stopords', () => {
let stopWords = {'en': ['word1', 'stopword2', 'thirdword']}
let mockedWords = { 'word1': 1, 'stays': 1, "'stopword2": 1,"'thirdword":1};
let mockedLanguage = 'en';
let expectedResult = {
'stays': 1
};
let result = removeStopwords( mockedWords, mockedLanguage );
expect( result ).toEqual( expectedResult );
} );
} );
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not really understand what you mean by this:
right now this function depends on (tightly coupled with) the actual words. so in your tests you have to use one of these words from the original stopwords.
I dont know why the other tests that are not related fail... |
I have also no Idea why the check works now... However now it seems fine |