refactored remove rule and added updateRule

Author: ilanskogan

docs/engine.md

@@ -44,6 +44,8 @@ undefined facts as `undefined`.  (default: false)
 
 `pathResolver` - Allows a custom object path resolution library to be used. (default: `json-path` syntax). See [custom path resolver](./rules.md#condition-helpers-custom-path-resolver) docs.
 
+`ruleKeyField` - By default this field is set to 'id', this field is the rule property that will be used when removing a rule by a key string or when updating rule
+
 ### engine.addFact(String id, Function [definitionFunc], Object [options])
 
 ```js
@@ -73,6 +75,7 @@ engine.removeFact('speed-of-light')
 ### engine.addRule(Rule instance|Object options)
 
 Adds a rule to the engine.  The engine will execute the rule upon the next ```run()```
+if the rule doesn't have value set at rule[engine.ruleKeyField], a random string will be set instead.
 
 ```js
 let Rule = require('json-rules-engine').Rule
@@ -102,6 +105,22 @@ engine.addRule(rule)
 
 //remove it
 engine.removeRule(rule)
+//or
+engine.removeRule(rule.id)
+```
+
+ ### engine.updateRule(Rule instance|Object options)
+
+ Updates a rule in the engine.
+
+```javascript
+// adds a rule
+let rule = new Rule()
+engine.addRule(rule)
+rule.conditions.all = []
+
+//update it
+engine.updateRule(rule)
 ```
 
 ### engine.addOperator(String operatorName, Function evaluateFunc(factValue, jsonValue))

src/engine.js

@@ -25,6 +25,7 @@ class Engine extends EventEmitter {
     this.operators = new Map()
     this.facts = new Map()
     this.status = READY
+    this.ruleKeyField = options.ruleKeyField || 'id'
     rules.map(r => this.addRule(r))
     defaultOperators.map(o => this.addOperator(o))
   }
@@ -47,27 +48,44 @@ class Engine extends EventEmitter {
     } else {
       if (!Object.prototype.hasOwnProperty.call(properties, 'event')) throw new Error('Engine: addRule() argument requires "event" property')
       if (!Object.prototype.hasOwnProperty.call(properties, 'conditions')) throw new Error('Engine: addRule() argument requires "conditions" property')
-
       rule = new Rule(properties)
     }
     rule.setEngine(this)
-
+    if (!rule[this.ruleKeyField]) rule[this.ruleKeyField] = '_' + Math.random().toString(36).substr(2, 9)
     this.rules.push(rule)
     this.prioritizedRules = null
     return this
   }
 
   /**
-   * Remove a rule from the engine
+   * update a rule in the engine
    * @param {object|Rule} rule - rule definition. Must be a instance of Rule
    */
-  removeRule (rule) {
-    if ((rule instanceof Rule) === false) throw new Error('Engine: removeRule() rule must be a instance of Rule')
+  updateRule (rule) {
+    const ruleIndex = this.rules.findIndex(ruleInEngine => ruleInEngine[this.ruleKeyField] === rule[this.ruleKeyField])
+    if (ruleIndex > -1) {
+      this.rules.splice(ruleIndex, 1)
+      this.addRule(rule)
+    } else {
+      throw new Error('Engine: updateRule() rule not found')
+    }
+  }
 
-    const index = this.rules.indexOf(rule)
-    if (index === -1) return false
-    this.prioritizedRules = null
-    return Boolean(this.rules.splice(index, 1).length)
+  /**
+   * Remove a rule from the engine
+   * @param {object|Rule||string} rule - rule definition. Must be a instance of Rule
+   */
+  removeRule (rule) {
+    if (!(rule instanceof Rule)) {
+      this.rules = this.rules.filter(ruleInEngine => ruleInEngine[this.ruleKeyField] !== rule)
+      this.prioritizedRules = null
+      return Boolean(this.rules.length)
+    } else {
+      const index = this.rules.indexOf(rule)
+      if (index === -1) return false
+      this.prioritizedRules = null
+      return Boolean(this.rules.splice(index, 1).length)
+    }
   }
 
   /**

test/engine.test.js

@@ -84,6 +84,17 @@ describe('Engine', () => {
     })
   })
 
+  describe('updateRule()', () => {
+    it('updates rule', () => {
+      const rule = new Rule(factories.rule())
+      engine.addRule(rule)
+      expect(engine.rules[0].conditions.all.length).to.equal(2)
+      rule.conditions = { all: [] }
+      engine.updateRule(rule)
+      expect(engine.rules[0].conditions.all.length).to.equal(0)
+    })
+  })
+
   describe('removeRule()', () => {
     describe('rule instance', () => {
       it('removes the rule', () => {
@@ -96,14 +107,6 @@ describe('Engine', () => {
       })
     })
 
-    describe('required fields', () => {
-      it('.conditions', () => {
-        expect(() => {
-          engine.removeRule([])
-        }).to.throw(/Engine: removeRule\(\) rule must be a instance of Rule/)
-      })
-    })
-
     it('can only remove added rules', () => {
       expect(engine.rules.length).to.equal(0)
       const rule = new Rule(factories.rule())
@@ -118,6 +121,14 @@ describe('Engine', () => {
       engine.removeRule(rule)
       expect(engine.prioritizedRules).to.equal(null)
     })
+    it('removes rule based on ruleKey', () => {
+      const rule = new Rule(factories.rule())
+      engine.addRule(rule)
+      expect(engine.rules.length).to.equal(1)
+      engine.removeRule(rule.id)
+      expect(engine.rules.length).to.equal(0)
+      expect(engine.prioritizedRules).to.equal(null)
+    })
   })
 
   describe('addOperator()', () => {