Fix bug in Expression.simplify

The original implementation of Expression._combineLikeTerms was written
when Expressions were always simplified. In some cases where like-terms
show up multiple times and are scattered, terms would be missed because
I was splicing on the array that was being iterated through (which is
probably bad in general). It’s been rewritten to keep track of which
terms have already been encountered.

E.g., this used to fail:

var exp = algebra.parse("x + y + 3");
var unsimplified = exp.pow(3, false);
unsimplified.toString();

// xxx + xyy + yxx + yxy + yyx + yyy + xxy + xyx + 3yx + 3xy + 3xy +
3yy + 3xx + 3xx + 3yy + 3yx + 3yy + 3yx + 3xx + 3xy + 3 * 3x + 3 * 3y +
3 * 3x + 3 * 3y + 3 * 3x + 3 * 3y + 3 * 3 * 3

var simplified = unsimplified.simplify();
simplified.toString();
// x^3 + y^3 + 2x^2y + y^2x + x^2y + 2y^2x + 6y^2 + 9x^2 + 3y^2 + 3xy +
15yx + 27x + 18y + 9y + 27

Notice the 2x^2y and x^2y terms are not combined when they should be.

This now works correctly:

var simplified = unsimplified.simplify();
simplified.toString();
// x^3 + y^3 + 3x^2y + 3y^2x + 9x^2 + 9y^2 + 18xy + 27x + 27y + 27

src/expressions.js

@@ -38,11 +38,11 @@ Expression.prototype.simplify = function() {
         copy.terms[i] = copy.terms[i].simplify();
     }
 
+    copy._sort();
     copy._combineLikeTerms();
     copy._moveTermsWithDegreeZeroToConstants();
     copy._removeTermsWithCoefficientZero();
     copy.constants = (copy.constant().valueOf() === 0 ? [] : [copy.constant()]);
-    copy._sort();
 
     return copy;
 };
@@ -211,11 +211,12 @@ Expression.prototype.pow = function(a, simplify) {
             }
 
             copy._sort();
-            return copy;
         }
     } else {
         throw "InvalidArgument";
     }
+
+    return (simplify ? copy.simplify() : copy);
 };
 
 Expression.prototype.eval = function(values, simplify) {
@@ -315,20 +316,40 @@ Expression.prototype._removeTermsWithCoefficientZero = function() {
 };
 
 Expression.prototype._combineLikeTerms = function() {
+    function alreadyEncountered(term, encountered) {
+        for (var i = 0; i < encountered.length; i++) {
+            if (term.canBeCombinedWith(encountered[i])) {
+                return true;
+            }
+        }
+
+        return false;
+    }
+
+    var newTerms = [];
+    var encountered = [];
+
     for (var i = 0; i < this.terms.length; i++) {
         var thisTerm = this.terms[i];
 
-        for (var j = i + 1; j < this.terms.length; j++) {
-            var thatTerm = this.terms[j];
+        if (alreadyEncountered(thisTerm, encountered)) {
+            continue;
+        } else {
+            for (var j = i + 1; j < this.terms.length; j++) {
+                var thatTerm = this.terms[j];
 
-            if (thisTerm.canBeCombinedWith(thatTerm)) {
-                thisTerm = thisTerm.add(thatTerm);
-                this.terms[i] = thisTerm;
-                this.terms.splice(j, 1);
+                if (thisTerm.canBeCombinedWith(thatTerm)) {
+                    thisTerm = thisTerm.add(thatTerm);
+                }
             }
+
+            newTerms.push(thisTerm);
+            encountered.push(thisTerm);
         }
+
     }
 
+    this.terms = newTerms;
     return this;
 };
 

test/expression-spec.js

@@ -762,6 +762,14 @@ describe("Expression simplification", function() {
 
         expect(ans.toString()).toEqual("25");
     });
+
+    it("should properly simplify expressions where terms show up >= 2 times", function() {
+        var exp = new Expression("x").add("y").add(3);
+        var unsimplified = exp.pow(3, false);
+        var simplified = unsimplified.simplify();
+
+        expect(simplified.toString()).toEqual("x^3 + y^3 + 3x^2y + 3y^2x + 9x^2 + 9y^2 + 18xy + 27x + 27y + 27");
+    });
 });
 
 describe("Expression summation", function() {