Conversation
mowolf
commented
May 27, 2018
mowolf
commented
- refactured removeStpwords
|
|
||
| export const removeStopwords = (wordCountObject, language) => { | ||
| var stopwords = STOPWORDS[language]; | ||
| var filteredObject = Object.assign(wordCountObject); |
There was a problem hiding this comment.
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.
but this is not what this line does. check Object.assign method documentation.
There was a problem hiding this comment.
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.
as discussed, this is risky as it mutates the original object.
There was a problem hiding this comment.
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', () => { |
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.
better to save STOPWORDS in a separate file as it is localization not code constants
| } | ||
|
|
||
| export const removeStopwords = (wordCountObject, language) => { | ||
| var stopwords = STOPWORDS[language]; |
There was a problem hiding this comment.
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
STOPWORDSduring the test (which is a bit complicated) - as
STOPWORDSis 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.
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 |
2 participants
