Skip to content
This repository has been archived by the owner on Dec 10, 2019. It is now read-only.

Remove stopwords refacture #4

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Remove stopwords refacture #4

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
03ae974
moved removeStopwords
mowolf May 25, 2018
4272c97
refactured
mowolf May 26, 2018
ffb883f
Added refacturing and testing
mowolf May 27, 2018
9b9172f
added suggestion
mowolf May 27, 2018
97a025f
duplicated object
mowolf May 27, 2018
28eab83
renamed variables
mowolf May 27, 2018
cb90bb5
Merge branch 'master' into removeStopwords-Refacture
mowolf May 27, 2018
deb2721
updated & fixed comments
mowolf Jun 2, 2018
6ab66a1
pulled newest master
mowolf Jun 2, 2018
d2d1e48
used own file for stopwrds
mowolf Jun 2, 2018
d2a640b
Merge branch 'master' into removeStopwords-Refacture
mowolf Jun 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/constants.js
View file
Open in desktop

Large diffs are not rendered by default.

38 changes: 38 additions & 0 deletions src/removeStopwords.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
// 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';
Copy link
Contributor

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jep


const filterWords = (wordCountObject, wordsToFilterOut) => {
for (var key in wordCountObject) {
var containsWord = (wordsToFilterOut.indexOf(key) > -1);
if (containsWord) {
delete wordCountObject[key];
Copy link
Contributor

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.

Copy link
Member Author

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.

}
}
return wordCountObject;
}

export const removeStopwords = (wordCountObject, language) => {
var stopwords = STOPWORDS[language];
Copy link
Contributor

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 );

  } );
} );

Copy link
Member Author

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.

var filteredObject = Object.assign(wordCountObject);
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Member Author

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

return filterWords(filteredObject, stopwords)

// TODO: MOVE HTML
// // get element
// div = document.getElementById("wordsByUsage"+i);
// // remove old content
// div.innerHTML = "";
// // add new content
// if (filteredWords[i].length > 29) {
// max = 30;
// } else {
// max = filteredWords[i].length;
// }
// var mostUsedHTML ="";
// for (var j = 0; j < max; j++) {
// mostUsedHTML = mostUsedHTML + "<p>" + filteredWords[i][j][0].substring(1) +" - "+ Math.round(filteredWords[i][j][1]/totalWords*1000)/10 + "%</p>";
// }
// div.innerHTML = mostUsedHTML;

}
22 changes: 22 additions & 0 deletions src/removeStopwords.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import { removeStopwords } from './removeStopwords';

describe( 'removeStopwords', () => {

it( 'should be function', () => {
expect( typeof removeStopwords ).toBe( 'function' );
} );

it( 'should remove stopords', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stopords -> stopWords

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jep

let mockedWords = { 'i': 1, 'asdasd': 1, "'ll": 1,"'tis":1,"'twas":1};
let mockedLanguage = 'en';
let expectedResult = {
'asdasd': 1
};

let result = removeStopwords( mockedWords, mockedLanguage );
expect( result ).toEqual( expectedResult );

} );


} );