Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #19: Implementing reading time feature and tester file + update package.json #131

Closed
wants to merge 1 commit into from

Conversation

yohan-choi-dev
Copy link

I added basic features for text analysis. In the beginning, my goal was to add reading-time features. However, I realized that word count, the level of writing (readability based on Flesch-Kincaid grade level) and reading-time are highly connected.

For instance, the formula for readability is formula_image
To get the level of writing, we need the number of words, and the formula to get reading time is the total number of words / word per minute. Besides the reading-level library that I use to get Flesch_Kincaid Grade, it also returns the count of words.

Since the three basic features are highly coupled, I think that coupling them together as one class will be better than implementing three different functions. I tried to implement the OOP concept into the function. Because an ES6 class does not fully support encapsulation, I used ES5 syntax with the closures concept in JavaScript to implement private members.

The analyzeText() function return analysis object that contains wordCount, readability, readingTime. The analyzeText() function contains two different public query functions, getAnalysis(), and getAsyAnalsis(). In case of using promise channing, getAsyAnalsis() can be used.

The tester file provides queries to check if functions work well, but it will need more functionality. Plus, I updated the package.json file since it caused an error on the previous pull-request

I'm not familiar with JavaScript so if you have any opinions, feel free to share your opinions. I will refect the opinions.

@manekenpix manekenpix self-requested a review November 9, 2019 22:07
Copy link
Member

@manekenpix manekenpix left a comment

Choose a reason for hiding this comment

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

Try to get rid of this:

Screenshot from 2019-11-09 17-09-13

It has a bunch of changes that are already in master.

@manekenpix manekenpix added on-board area: back-end type: test Creation and development of test labels Nov 9, 2019
analyzeText(text1).getAsyAnalysis()
.then((data) => console.log(data))
.catch((err) => console.log(err));
});
Copy link
Contributor

Choose a reason for hiding this comment

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

As we are using JEST as our testing framework, I think these tests should also be written with expect and await.
This is a great resource for testing promises with Jest.

Copy link
Author

Choose a reason for hiding this comment

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

which means we are allowed to use ES7 Syntax in our code? Only test files or other source files?

Copy link
Contributor

Choose a reason for hiding this comment

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

async/await are fine in src and tests, yes.

Copy link
Author

Choose a reason for hiding this comment

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

I added the async/await test unit.

@humphd
Copy link
Contributor

humphd commented Nov 16, 2019

Merged in 680b8a9

@humphd humphd closed this Nov 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: back-end type: test Creation and development of test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants