jiggzson / nerdamer

a symbolic math expression evaluator for javascript
http://www.nerdamer.com
MIT License
517 stars 82 forks source link

Bug in equals method of symbol #479

Closed Saikedo closed 5 years ago

Saikedo commented 5 years ago

There seems to be a bug in equals method of the symbol. Here is what I noticed

The expression nerdamer("sin(((pi)\^2)+pi)").evaluate().text() returns "0" which is wrong. I was trying to debug this issue when I noticed that the following if statement is evaluated as "true" which is not correct.

if (symbol.equals('pi'))
                    retval = new Symbol(0);

So the symbol here was pi+pi^2 but the equals shows like it is equal to pi.

And here is the code from equals but I am not sure how his problem can be fixed. Any ideas?

equals: function (symbol) { if (!isSymbol(symbol)) symbol = new Symbol(symbol); return this.value === symbol.value && this.power.equals(symbol.power) && this.multiplier.equals(symbol.multiplier); },

jiggzson commented 5 years ago

Simple fix. Replace symbol.equals with

        equals: function (symbol) {
            if (!isSymbol(symbol))
                symbol = new Symbol(symbol);
            return this.value === symbol.value && this.power.equals(symbol.power) 
                    && this.multiplier.equals(symbol.multiplier) 
                    && this.group === symbol.group;
        }

Forgetting to check that the groups match as well.

jiggzson commented 5 years ago

@Saikedo Do you think that you'll be able to create some test cases for this one?

Saikedo commented 5 years ago

@jiggzson I sure can. Can you tell me a bit about what scenarios would be best to test here? Also, where do we write the tests for Nerdamer?

Saikedo commented 5 years ago

@jiggzson Also I already added the fix in my version that I will make a push for soon so no need for you to add it as well.

jiggzson commented 5 years ago

@Saikedo, thanks. I appreciate that. Tests are in the /spec folder. I guess it would make sense that tests for equals go in the core.spec folder. Also I've already added a fix for this as well so just pull in the changes before committing.