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

Added RuleBase class to allow easier use of the library #75

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

Conversation

skanderturki
Copy link

Hi seb,

I have added the class (RuleBase) along with an example that illustrates how to use the class. I hope you find this useful.

Thanks

return this.rules[i].result;
}
// If no rule evaluated to true then there is a problem with the model.
return(-1);
Copy link
Owner

Choose a reason for hiding this comment

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

parnthesis around a return value?

Copy link
Author

Choose a reason for hiding this comment

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

you're right

* initialized to "false".
*/
addRule(evaluator, result) {
this.rules.push( { evaluator: evaluator, evaluation: false, result: result} );
Copy link
Owner

Choose a reason for hiding this comment

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

will a max value for rules be useful here? just to make sure no one is creating something uncomputable

Copy link
Author

Choose a reason for hiding this comment

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

I agree

* was evaluated to true.
*/
getResult(){
for(let i = 0; i < this.rules.length; i++){
Copy link
Owner

Choose a reason for hiding this comment

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

maybe a question of style: amaybe consider using .map, filter and a length check

Additionally this makes it possible to only return something from one line.

* It looks for the rule (should be only one if the model is correct) that
* was evaluated to true.
*/
getResult(){
Copy link
Owner

Choose a reason for hiding this comment

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

naming: getResult .. tehse days I try not to write get ... there are actually getters for that ...

Copy link
Author

Choose a reason for hiding this comment

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

ok I'll modify the name, maybe to evaluation()

@@ -1,6 +1,6 @@
The MIT License (MIT)

Copyright (c) 2015 [these people](https://github.com/sebs/es6-fuzz/graphs/contributors)
Copyright (c) 2015 [these people](https://github.com/skanderturki/js-fuzzy-logic/graphs/contributors)
Copy link
Owner

Choose a reason for hiding this comment

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

I think the link in my repo represents very well who contributed.

Copy link
Author

Choose a reason for hiding this comment

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

ok, I'm not used to this, thanks

Copy link
Owner

Choose a reason for hiding this comment

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

no problem ;)

@@ -1,4 +1,4 @@
# es6-fuzz
# js-fuzzy-logic ( forked from: es6-fuzz )
Copy link
Owner

Choose a reason for hiding this comment

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

this one needs to be the old value to be merged.

@@ -21,12 +21,6 @@ Supported fuzzyfiers
* [api docs](http://sebs.github.io/es6-fuzz)
* [changelog](https://github.com/sebs/es6-fuzz/blob/master/docs/CHANGELOG.md)

## Install and Usage
Copy link
Owner

Choose a reason for hiding this comment

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

this i like to get back ;)

@@ -0,0 +1,70 @@
const Logic = require('./lib/logic')
Copy link
Owner

Choose a reason for hiding this comment

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

please move this to examples folder,

Copy link
Author

Choose a reason for hiding this comment

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

ok got it

@sebs
Copy link
Owner

sebs commented Jan 22, 2023

I am happy to merge as soon as the the comments are addressed. I think its mostly cosmetics.

Thank you for the time!
S.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants